<attachment> shouldn't use "user-select: all"
Created attachment 248184 [details] Patch
<rdar://problem/20085435>
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
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
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
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
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.
(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.
http://trac.webkit.org/changeset/181418
And http://trac.webkit.org/changeset/181420