Bug 13624 - REGRESSION: can select text of an input button
Summary: REGRESSION: can select text of an input button
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-05-08 04:22 PDT by Eric Seidel (no email)
Modified: 2012-03-23 17:54 PDT (History)
9 users (show)

See Also:


Attachments
picture showing change. TOT in foreground (11.63 KB, image/png)
2007-05-08 04:24 PDT, Eric Seidel (no email)
no flags Details
Test case for buttons and inline-block (492 bytes, text/html)
2007-05-12 06:17 PDT, mitz
no flags Details
Proposed Patch (27.92 KB, patch)
2012-03-14 04:38 PDT, Parag Radke
no flags Details | Formatted Diff | Diff
Updated Patch (28.90 KB, patch)
2012-03-16 00:44 PDT, Parag Radke
no flags Details | Formatted Diff | Diff
Updated Patch to fix merge conflicts (28.92 KB, patch)
2012-03-16 03:07 PDT, Parag Radke
no flags Details | Formatted Diff | Diff
Updated Patch for chromium pixel test failures (29.84 KB, patch)
2012-03-20 23:20 PDT, Parag Radke
no flags Details | Formatted Diff | Diff
Updated patch (25.77 KB, patch)
2012-03-23 11:08 PDT, Parag Radke
no flags Details | Formatted Diff | Diff
Updated patch (23.04 KB, patch)
2012-03-23 12:48 PDT, Parag Radke
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (18.37 MB, application/zip)
2012-03-23 14:47 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-05-08 04:22:32 PDT
Um.  This is kinda cool.  But a regression none the less.

You can reproduce on this bugzilla page by clicking on the space to the right of a button and then drag-selecting left.
Comment 1 Eric Seidel (no email) 2007-05-08 04:24:15 PDT
Actually the selection behavior seems "correct", the selection rect is just smaller than expected based on the behavior in the previous safari.
Comment 2 Eric Seidel (no email) 2007-05-08 04:24:52 PDT
Created attachment 14410 [details]
picture showing change.  TOT in foreground
Comment 3 mitz 2007-05-12 06:17:53 PDT
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.
Comment 4 mitz 2009-06-17 00:28:47 PDT
<rdar://problem/6977976>
Comment 5 Konstantin Shcheglov 2011-09-16 11:50:29 PDT
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.
Comment 6 Alexey Proskuryakov 2011-09-16 16:35:04 PDT
In other words, text in a button in non-editable content should never have a selection of its own painted, correct?
Comment 7 Ryosuke Niwa 2011-09-16 16:36:07 PDT
(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.
Comment 8 Parag Radke 2012-03-14 04:38:47 PDT
Created attachment 131825 [details]
Proposed Patch

Patch to avoid selection background for the button text.
Comment 9 WebKit Review Bot 2012-03-14 07:28:18 PDT
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
Comment 10 Parag Radke 2012-03-16 00:44:33 PDT
Created attachment 132220 [details]
Updated Patch

Patch to avoid selection background for the button text if button is inside non content editable area.
Comment 11 Parag Radke 2012-03-16 03:07:49 PDT
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 12 WebKit Review Bot 2012-03-16 04:17:48 PDT
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
Comment 13 Tony Chang 2012-03-19 14:08:56 PDT
(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?
Comment 14 Parag Radke 2012-03-20 23:20:38 PDT
Created attachment 132978 [details]
Updated Patch for chromium pixel test failures

Patch to avoid selection for the button text.
Comment 15 Ryosuke Niwa 2012-03-20 23:28:53 PDT
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.
Comment 16 Parag Radke 2012-03-21 06:49:32 PDT
(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.
Comment 17 Ryosuke Niwa 2012-03-21 09:58:03 PDT
(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.
Comment 18 Parag Radke 2012-03-23 11:08:43 PDT
Created attachment 133514 [details]
Updated patch

Updated patch with all review comments implemented
Comment 19 Ryosuke Niwa 2012-03-23 11:42:52 PDT
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.
Comment 20 Parag Radke 2012-03-23 12:48:16 PDT
Created attachment 133540 [details]
Updated patch

Updated patch with all review comments implemented
Comment 21 WebKit Review Bot 2012-03-23 13:32:53 PDT
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 22 WebKit Review Bot 2012-03-23 14:46:54 PDT
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
Comment 23 WebKit Review Bot 2012-03-23 14:47:05 PDT
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 24 Ryosuke Niwa 2012-03-23 17:54:08 PDT
Comment on attachment 133540 [details]
Updated patch

Clearing flags on attachment: 133540

Committed r111946: <http://trac.webkit.org/changeset/111946>
Comment 25 Ryosuke Niwa 2012-03-23 17:54:25 PDT
All reviewed patches have been landed.  Closing bug.