Opened 5 years ago

Closed 5 years ago

#11377 closed Task (fixed)

Unify internal representation of empty anchors using fake objects

Reported by: Piotrek Koszuliński Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.4.0
Component: General Version:
Keywords: Cc:

Description

Currently we use fake objects on Webkits only. This is driven by http://docs.ckeditor.com/#!/api/CKEDITOR.plugins.link-property-fakeAnchor property.

Since we've got problems with DnD of empty anchors (#11140) and branched code means harder maintenance and bigger code size, we could simplify all that by using fake objects on every browser.

Possible threats

  • Empty anchor represented by fake object may not look good depending on font size of a surrounding text. A sample with different text styling should be created for easy testing.
  • New issues may be introduced while simplifying code. To be tested:
    • data processing,
    • creating anchor and selection after creation,
    • discovering anchors for options in link dialog (with link type set to anchor),
    • removing anchor,
    • options in context menu,
    • editing anchors (+dbclick),
    • and finally DnD.

Fixes

Change History (10)

comment:1 Changed 5 years ago by Piotrek Koszuliński

I pushed t/11377 with a quick prototype. It looks that visually everything is ok. I found one issue though - on IEs anchor is not selected after it's created.

In any case - further test are needed.

comment:2 Changed 5 years ago by Piotrek Koszuliński

PS. if first commit in t/11377 is not apparent you can check first to what values those three properties have been unified: https://github.com/cksource/ckeditor-dev/commit/098f1c48c545c7b1ab2ae42ff181bb7fd597579c#diff-d9e496f4ce47ad70bef0ba672713d3a1L265

comment:3 Changed 5 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4
Status: newconfirmed

comment:4 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:5 Changed 5 years ago by Marek Lewandowski

Status: assignedreview

Conflicts merged, rebased to master at t/11377 dev and created t/11377 at tests. Added fixes for IE selection, and small update in docs. One TC needed to be adjusted, and be aware that IE11 puts extra br in particular TC.

For this ticket i did testing in IE8 - IE11 all of them works ok with fake anchors. There were two major issues at the begining:

IE (general): When anchor with innertext were fully selected (i.e. <a name="foo">[bar]</a>, then edit dialog would not fetch its name. IE (general): After updating anchor name, caret was placed at the very beginning of the document.

Last edited 5 years ago by Marek Lewandowski (previous) (diff)

comment:6 Changed 5 years ago by Piotrek Koszuliński

Note that fake object wasn't used on FF too, so it should be tested as well.

comment:7 Changed 5 years ago by Marek Lewandowski

My bad, i did testing for chrome, ff26, ie8-ie11. I've skipped only opera (since it's still blink), and safari.

comment:8 Changed 5 years ago by Marek Lewandowski

Once again, conflicts merged and rebased to major at t/11377 dev and tests.

comment:9 Changed 5 years ago by Olek Nowodziński

Status: reviewreview_passed

I rebased branches once again, added a single commit to dev and force-pushed all stuff to remotes.

I checked Chrome, FF, IE(8,9,11) - manually and automatically. Everything seems OK for me. If something's broken we're gonna figure it out before release. Hopefully :P

I'd squash those two commits

"Moved variable initialization."
"Code simplification."

since they don't introduce any new logic. They should be merged with corresponding ancestors before the branch is majorized.

comment:10 Changed 5 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Majorized with git:5875583254 at dev and 7f8d249449ec10 at tests.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy