Bug 52742 – Stop instantiating legacy editing positions in FocusController::advanceFocusInDocumentOrder

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

We're currently in the process of removing all instantiations of legacy editing positions.  FocusController::advanceFocusInDocumentOrder currently instantiates two legacy editing positions in the line:
    if (caretBrowsing) {
        VisibleSelection newSelection(Position(node, 0), Position(node, 0), DOWNSTREAM);
        if (frame->selection()->shouldChangeSelection(newSelection))
            frame->selection()->setSelection(newSelection);
    }

They should be calling one of helper functions in Position.h or htmlediting.h.

[Ryosuke Niwa]

I think the right thing to do here is to call firstPositionInOrBeforeNode.

This function produces a position [node, 0] when node can have children for editing (div, blockquote, span, etc...) and before node when node can't have children for editing (br, img, etc...).

Could someone from GTK comment on whether this is right behavior or not?

[Mario Sanchez Prada]

(In reply to comment #1)
> I think the right thing to do here is to call firstPositionInOrBeforeNode.
> 
> This function produces a position [node, 0] when node can have children for editing (div, blockquote, span, etc...) and before node when node can't have children for editing (br, img, etc...).
> 
> Could someone from GTK comment on whether this is right behavior or not?

I've some doubts whether the right thing to do would be just to call firstPositionInNode(node) instead of your suggestion, but then I've tried it out and run the unit and layout tests and it seems everything is fine with firstPositionInOrBeforeNode(node).

So guess you're right and thus that your patch is the right way to go.

[Ryosuke Niwa]

(In reply to comment #2)
> I've some doubts whether the right thing to do would be just to call firstPositionInNode(node) instead of your suggestion, but then I've tried it out and run the unit and layout tests and it seems everything is fine with firstPositionInOrBeforeNode(node).

Calling firstPositionInNode won't make any sense if editing can ignore contents for the node.  e.g. if node was br, img, textarea, etc... it doesn't make any sense to have a position inside that node.  Is there any guarantee that node is always a container and the contents are not ignored by editing?  That's a requirement for us to be calling firstPositionInNode.

[Mario Sanchez Prada]

(In reply to comment #3)
> (In reply to comment #2)
> > I've some doubts whether the right thing to do would be just to call firstPositionInNode(node) instead of your suggestion, but then I've tried it out and run the unit and layout tests and it seems everything is fine with firstPositionInOrBeforeNode(node).
> 
> Calling firstPositionInNode won't make any sense if editing can ignore contents for the node.  e.g. if node was br, img, textarea, etc... it doesn't make any sense to have a position inside that node.  Is there any guarantee that node is always a container and the contents are not ignored by editing?  That's a requirement for us to be calling firstPositionInNode.

If I could have any doubt wrt this stuff, now you've completely dissipated them once and for all. Thanks for the explanation, I appreciate that.

[Ryosuke Niwa]

Mario, are you working on this bug?

[Mario Sanchez Prada]

(In reply to comment #5)
> Mario, are you working on this bug?

Not at the moment, sorry. I've started to take a look into it but then I got distracted by other bugs and, I've to recognize, I forgot about this one.

Anyway, feel free to make the change yourself if you want. As you can read in comment #2 I tried that out some weeks ago and seemed to work fine, while not breaking any test.

If not, I guess I could take care of this when I finish my current tasks, but things this month are piling up so quickly I can't tell for sure...

Again, sorry for overlooking this.

[Ryosuke Niwa]

Created attachment 84539 [details]
fixes the bug

[Ryosuke Niwa]

Could someone from Qt / GTK ports verify that this patch doesn't cause a regression by running layout tests?

[Philippe Normand]

I'll check this on GTK+, sorry for  the delay!

[Philippe Normand]

I checked the editing, accessibility and unit tests on GTK without spotting a failure

[Ryosuke Niwa]

Committed r80298: <http://trac.webkit.org/changeset/80298>