Opened 6 years ago

Closed 6 years ago

#11042 closed Bug (fixed)

Selection.selectRanges selects contents of a non-editable element

Reported by: Piotrek Koszuliński Owned by: Marek Lewandowski
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.3.1
Component: Core : Selection Version:
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

  1. Go to placeholder sample.
  2. Select placeholder.
  3. Click bold button.
  4. Click "strong" in elements path
  • Expected: placeholder should be selected with fake selection.
  • Actual: selection is anchored inside widget element.

Elements path uses the selection#selectElement method which does nothing more than selectRanges, so the issue is inside selectRanges or in browsers (less likely).


It also concerns IE8, but even more often. In #10887 we needed to disable two tests:

Change History (8)

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

Description: modified (diff)
Status: newconfirmed

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

Priority: NormalHigh

comment:3 Changed 6 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:4 Changed 6 years ago by Marek Lewandowski

Status: assignedreview

Commited to t/11042 dev and t/11042 in tests.

The only remark is that with that change, clicking parent elements ( to nested widget ) seems not to respond / be broken.

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

Status: reviewreview_failed

I rebased dev branch and pushed several (minor) improvements there.

Although the code is just alright, there are several issues in tests:

  • Tests should be a part of dt since they touch a very general problem in core.
  • Tests should have user-friendly names, unlike "test1".
  • Empty <meta name="cktester-plugins" content=""> in head.
  • id="outer_p" and class="foo" are obsolete and not utilized.
  • Tcs are a little bit obscure:
    <p id="outer_p">aa <strong><span class="foo"><b><i contenteditable="false">bb</i></b></span></strong> cc</p>
    
    Why not:
    <p>aa <strong><i contenteditable="false">bb</i></strong> cc</p>
    
    It's clean, isn't it ? :)
  • There are no tests for false positives, i.e.:
    <p>aa <strong>oops there's text inside<i contenteditable="false">bb</i></strong> cc</p>
    
    Perhaps there are other cases on which that fix may fail?
  • Error messages should be inversed:
    assert.isTrue( !!sel.isFake, 'Produced selection is not fake' );
    
    to
    
    assert.isTrue( !!sel.isFake, 'Produced selection is fake' );
    

comment:6 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

Correct :) html might be a little bit cleaner, because of moving to seleciton.fake, i changed [i][b] tags to strongs/em to avoid config change in selection.fake tests.

in addition: assert message was changed to

assert.isTrue( !!sel.isFake, 'Produced selection should be fake' );

since it's raised when selection is not faked. I did not add extra test cases, since other cases were already handled correctly when i began this ticket.

Tests updated in t/11042.

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

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

Status: reviewreview_passed

I pushed couple of commits which:

  1. simplified tests,
  2. added false positive test,
  3. shortened dev code.

And I squashed other commits which didn't contain any interesting history.

Also, I checked regressions mentioned in this ticket's description, but those tests were still broken so I created #11242.

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

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:eaa4592 on dev and 631db74 on tests.

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