Bug 52742 - Stop instantiating legacy editing positions in FocusController::advanceFocusInDocumentOrder
Summary: Stop instantiating legacy editing positions in FocusController::advanceFocusI...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 52099
  Show dependency treegraph
 
Reported: 2011-01-19 14:12 PST by Ryosuke Niwa
Modified: 2011-03-03 16:49 PST (History)
8 users (show)

See Also:


Attachments
fixes the bug (1.61 KB, patch)
2011-03-03 02:19 PST, Ryosuke Niwa
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-01-19 14:12:19 PST
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.
Comment 1 Ryosuke Niwa 2011-01-19 14:14:42 PST
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?
Comment 2 Mario Sanchez Prada 2011-01-20 03:01:42 PST
(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.
Comment 3 Ryosuke Niwa 2011-01-20 07:35:04 PST
(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.
Comment 4 Mario Sanchez Prada 2011-01-20 08:45:05 PST
(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.
Comment 5 Ryosuke Niwa 2011-03-03 01:07:08 PST
Mario, are you working on this bug?
Comment 6 Mario Sanchez Prada 2011-03-03 02:09:01 PST
(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.
Comment 7 Ryosuke Niwa 2011-03-03 02:19:29 PST
Created attachment 84539 [details]
fixes the bug
Comment 8 Ryosuke Niwa 2011-03-03 02:20:31 PST
Could someone from Qt / GTK ports verify that this patch doesn't cause a regression by running layout tests?
Comment 9 Philippe Normand 2011-03-03 07:00:52 PST
I'll check this on GTK+, sorry for  the delay!
Comment 10 Philippe Normand 2011-03-03 07:20:29 PST
I checked the editing, accessibility and unit tests on GTK without spotting a failure
Comment 11 Ryosuke Niwa 2011-03-03 16:49:37 PST
Committed r80298: <http://trac.webkit.org/changeset/80298>