Bug 142453 – <attachment> shouldn't use "user-select: all"

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

<attachment> shouldn't use "user-select: all"

[Tim Horton]

Created attachment 248184 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/20085435>

[Build Bot]

Comment on attachment 248184 [details]
Patch

Attachment 248184 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4673194791272448

New failing tests:
fast/attachment/attachment-select-on-click-inside-user-select-all.html
fast/attachment/attachment-select-on-click.html

[Build Bot]

Created attachment 248185 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 248184 [details]
Patch

Attachment 248184 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6024353921630208

New failing tests:
fast/attachment/attachment-select-on-click-inside-user-select-all.html
fast/attachment/attachment-select-on-click.html

[Build Bot]

Created attachment 248186 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Darin Adler]

Comment on attachment 248184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248184&action=review

Tests are failing on bot. Please don’t land until that’s resolved, of course.

> Source/WebCore/page/EventHandler.cpp:525
> +static VisibleSelection expandSelectionToRespectSelectOnMouseDown(Node* targetNode, const VisibleSelection& selection)

This function should take a reference, not a pointer, for targetNode. It unconditionally dereferences targetNode so it won’t work on null.

The name of this seems slightly too specific. Before it was specifically “user select all”, not it’s specifically “select on mouse down”, but really the function respects both. Is there wording that is brief, clear, and encompasses both? Or is “select on mouse down” already ambiguous in just the right way to encompass both?

> Source/WebCore/page/EventHandler.cpp:529
> +    if (targetNode->shouldSelectOnMouseDown())
> +        nodeToSelect = targetNode;

It’s wasteful to do this work when the USERSELECT_ALL paragraph might just overwrite this without ever looking at nodeToSelect. Worth rearranging so that’s not done.

> Source/WebCore/page/EventHandler.cpp:536
>      Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
> -    if (!rootUserSelectAll)
> +    if (!rootUserSelectAll && !nodeToSelect)
>          return selection;
> +    if (rootUserSelectAll)
> +        nodeToSelect = rootUserSelectAll;

When USERSELECT_ALL is enabled, this function returns the passed-in selection if it’s passed a node that returns false for shouldSelectOnMouseDown. But when it’s not, it instead returns a VisibleSelection of null. We should not have that kind of subtle difference in behavior. I suggest structuring the code so the early exit is:

    if (!nodeToSelect)
        return selection;

And it’s unconditional. The USERSELECT_ALL paragraph would be only:

    if (Node* rootUserSelectAll = Position::rootUserSelectAllForNode(&targetNode))
        nodeToSelect = rootUserSelectAll;

I think the function might read more clearly if we broke out another small function that decides which node should be selected, just to separate the selection machinery from the decision making more clearly and give more opportunities to use return in a more straightforward way.

[Tim Horton]

(In reply to comment #7)
> Comment on attachment 248184 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248184&action=review
> 
> Tests are failing on bot. Please don’t land until that’s resolved, of course.
> 
> > Source/WebCore/page/EventHandler.cpp:525
> > +static VisibleSelection expandSelectionToRespectSelectOnMouseDown(Node* targetNode, const VisibleSelection& selection)
> 
> This function should take a reference, not a pointer, for targetNode. It
> unconditionally dereferences targetNode so it won’t work on null.
> 
> The name of this seems slightly too specific. Before it was specifically
> “user select all”, not it’s specifically “select on mouse down”, but really
> the function respects both. Is there wording that is brief, clear, and
> encompasses both? Or is “select on mouse down” already ambiguous in just the
> right way to encompass both?

Yeah, I don't know what to do with the name. I was trying to be sufficiently vague that that name might work, but it certainly isn't great.

> > Source/WebCore/page/EventHandler.cpp:529
> > +    if (targetNode->shouldSelectOnMouseDown())
> > +        nodeToSelect = targetNode;
> 
> It’s wasteful to do this work when the USERSELECT_ALL paragraph might just
> overwrite this without ever looking at nodeToSelect. Worth rearranging so
> that’s not done.

Quite right.

> > Source/WebCore/page/EventHandler.cpp:536
> >      Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
> > -    if (!rootUserSelectAll)
> > +    if (!rootUserSelectAll && !nodeToSelect)
> >          return selection;
> > +    if (rootUserSelectAll)
> > +        nodeToSelect = rootUserSelectAll;
> 
> When USERSELECT_ALL is enabled, this function returns the passed-in
> selection if it’s passed a node that returns false for
> shouldSelectOnMouseDown. But when it’s not, it instead returns a
> VisibleSelection of null. We should not have that kind of subtle difference
> in behavior. I suggest structuring the code so the early exit is:
> 
>     if (!nodeToSelect)
>         return selection;
> 
> And it’s unconditional. The USERSELECT_ALL paragraph would be only:
> 
>     if (Node* rootUserSelectAll =
> Position::rootUserSelectAllForNode(&targetNode))
>         nodeToSelect = rootUserSelectAll;
> 
> I think the function might read more clearly if we broke out another small
> function that decides which node should be selected, just to separate the
> selection machinery from the decision making more clearly and give more
> opportunities to use return in a more straightforward way.

Yes! OK, that sounds good. I wasn't happy with the somewhat convoluted logic of this function, but wasn't sure how to make it better. That sounds like a good plan.

[Tim Horton]

http://trac.webkit.org/changeset/181418

[Tim Horton]

And http://trac.webkit.org/changeset/181420