Summary: | REGRESSION: can select text of an input button | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ap, dglazkov, inferno, mitz, parag, rniwa, scheglov, tony, webkit.review.bot | ||||||||||||||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2007-05-08 04:22:32 PDT
Actually the selection behavior seems "correct", the selection rect is just smaller than expected based on the behavior in the previous safari. Created attachment 14410 [details]
picture showing change. TOT in foreground
Created attachment 14516 [details]
Test case for buttons and inline-block
This is essentially a problem with replaced elements that have selectable content. The "regression" part is that buttons are implemented as such in TOT.
Just for records, for myself or may be for someone else. I've asked about this in IRC and here is my discussion with <rniwa>. <rniwa> scheglov: text inside button shouldn't be treated differently <rniwa> scheglov: it should act like any other text in the document <rniwa> scheglov: or else, <div contenteditable><button>hello</button></div> won't work properly <scheglov> rniwa: hm... So, do you have idea what https://bugs.webkit.org/show_bug.cgi?id=13624 means? <rniwa> scheglov: it's just rendering bug <rniwa> scheglov: selection isn't rendered properly inside/around button <scheglov> rniwa: Should be text on button selected? <rniwa> scheglov: yes <scheglov> OK, you told that it should be selected. <rniwa> scheglov: but when the entire button is selected, text selection shouldn't be doubly-rendered <rniwa> scheglov: I bet it's some quirk with theme/selection interaction <scheglov> So, "too much" variant is bug, right? <rniwa> scheglov: all of them are bug! <scheglov> rniwa: Oops... <scheglov> rniwa: I wonder what is correct rendering. <scheglov> rniwa: Ah... <scheglov> rniwa: So, "body" of button should be selected too? <rniwa> scheglov: yes <scheglov> rniwa: OK, now I understand. Thank you. <rniwa> scheglov: but if the entire button is selected, any selection inside the button should NOT be rendered <rniwa> scheglov: because that'll result in nested-selection rendering <rniwa> scheglov: on the other hand, it might be hard to render the case where just the button is selected <scheglov> rniwa: Yes, I see, in other case text is selected twice. <rniwa> scheglov: because we canonicalize position <scheglov> rniwa: Well, I don't understand this yet (hard and canonicalize), I need to read source. In other words, text in a button in non-editable content should never have a selection of its own painted, correct? (In reply to comment #6) > In other words, text in a button in non-editable content should never have a selection of its own painted, correct? Yes. In non-editable area, user shouldn't be able to select contents inside the button. Created attachment 131825 [details]
Proposed Patch
Patch to avoid selection background for the button text.
Comment on attachment 131825 [details] Proposed Patch Attachment 131825 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11957037 New failing tests: editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/3690719.html editing/selection/extend-by-word-002.html Created attachment 132220 [details]
Updated Patch
Patch to avoid selection background for the button text if button is inside non content editable area.
Created attachment 132239 [details]
Updated Patch to fix merge conflicts
Patch to avoid selection background for the button text if it is inside non editable area.
Comment on attachment 132239 [details] Updated Patch to fix merge conflicts Attachment 132239 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11960339 New failing tests: editing/selection/3690703-2.html editing/selection/select-all-004.html editing/pasteboard/input-field-1.html editing/selection/4895428-3.html editing/selection/3690703.html editing/selection/3690719.html (In reply to comment #12) > (From update of attachment 132239 [details]) > Attachment 132239 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11960339 > > New failing tests: > editing/selection/3690703-2.html > editing/selection/select-all-004.html > editing/pasteboard/input-field-1.html > editing/selection/4895428-3.html > editing/selection/3690703.html > editing/selection/3690719.html Looking at the checked in pngs, some of these look like progressions. E.g., editing/selection/3690703-2.html, editing/selection/3690703.html and editing/selection/3690719.html have buttons with selected text which I assume will no longer be selected after this change. The other 4 tests, I'm not so sure about. Do you know what the new results/diff looks like for them? Created attachment 132978 [details]
Updated Patch for chromium pixel test failures
Patch to avoid selection for the button text.
Comment on attachment 132978 [details] Updated Patch for chromium pixel test failures View in context: https://bugs.webkit.org/attachment.cgi?id=132978&action=review r- due to inefficient implementation. > Source/WebCore/rendering/RenderButton.h:51 > + virtual bool canBeSelectionLeaf() const { return node()->rootEditableElement(); } Finding rootEditableElement is very expensive. You should either call isContentEditable() or rendererIsEditable(). Call later if the layout is up-to-date whenever canBeSelectionLeaf is called. > Source/WebCore/rendering/RenderTextFragment.h:42 > + virtual bool canBeSelectionLeaf() const { return node()->rootEditableElement(); } Ditto. (In reply to comment #15) > Finding rootEditableElement is very expensive. You should either call isContentEditable() or rendererIsEditable(). Call later if the layout is up-to-date whenever canBeSelectionLeaf is called. To call isContentEditable(), i need to make sure this should not get call during layout is happening. I found needslayout() but I think it just tells weather present node's layout is complete or not but i have to check the complete layout is over or not. Can you please suggest the appropriate API i can use here. (In reply to comment #16) > (In reply to comment #15) > > Finding rootEditableElement is very expensive. You should either call isContentEditable() or rendererIsEditable(). Call later if the layout is up-to-date whenever canBeSelectionLeaf is called. > > To call isContentEditable(), i need to make sure this should not get call during layout is happening. I found needslayout() but I think it just tells weather present node's layout is complete or not but i have to check the complete layout is over or not. Can you please suggest the appropriate API i can use here. I'm not sure what you mean... isContentEditable only triggers style recalc. If you're in the middle of a layout, then the style must be up to date. Created attachment 133514 [details]
Updated patch
Updated patch with all review comments implemented
Comment on attachment 133514 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=133514&action=review > Source/WebCore/rendering/RenderButton.cpp:66 > + if (node()) > + return node()->isContentEditable(); > + > + return false; Ugh... this is a render object. You should be calling rendererIsEditable instead. r- because isContentEditable can trigger style recalc. It's also better to write this as return node() && node()->rendererIsEditable(). > Source/WebCore/rendering/RenderButton.h:43 > + virtual bool canBeSelectionLeaf() const; Please use OVERRIDE macro :) > Source/WebCore/rendering/RenderTextFragment.cpp:68 > + if (node()) > + return node()->isContentEditable(); > + > + return false; Ditto. > Source/WebCore/rendering/RenderTextFragment.h:42 > + virtual bool canBeSelectionLeaf() const; OVERRIDE. Created attachment 133540 [details]
Updated patch
Updated patch with all review comments implemented
Comment on attachment 133540 [details] Updated patch Rejecting attachment 133540 [details] from commit-queue. New failing tests: editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/3690719.html Full output: http://queues.webkit.org/results/12120820 Comment on attachment 133540 [details] Updated patch Attachment 133540 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12117822 New failing tests: editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/3690719.html Created attachment 133566 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 133540 [details] Updated patch Clearing flags on attachment: 133540 Committed r111946: <http://trac.webkit.org/changeset/111946> All reviewed patches have been landed. Closing bug. |