Opened 6 years ago

Closed 6 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 6 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 6 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 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4
Status: newconfirmed

comment:4 Changed 6 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:5 Changed 6 years ago by Marek Lewandowski

Status: assignedreview

Conflicts merged, rebased to t/11377. Added fixes for IE selection, and small update in docs. Branch updated does not break tests.

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.

Version 0, edited 6 years ago by Marek Lewandowski (next)

comment:6 Changed 6 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 6 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 6 years ago by Marek Lewandowski

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

comment:9 Changed 6 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 6 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 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy