Bug 54820 - REGRESSION: Knob of a disabled range control is draggable
: REGRESSION: Knob of a disabled range control is draggable
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P1 Normal
Assigned To: Nobody
data:text/html,<input type=range disa...
: Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-19 23:40 PST by Alexey Proskuryakov
Modified: 2011-07-27 02:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.32 KB, patch)
2011-07-23 06:12 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (7.89 KB, patch)
2011-07-26 01:08 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-02-19 23:40:42 PST
See bug URL. I can drag the knob as if the control was not disabled.

Has the range control been rewritten to use shadow DOM?
Comment 1 chris reiss 2011-02-21 11:18:08 PST
Having a look, same problem in QtTestBrowser, not in Chrome ...
Comment 2 chris reiss 2011-02-23 12:19:13 PST
looks like this fixes it.  how does one write a layout test for such a thing ?


Index: Source/WebCore/html/shadow/SliderThumbElement.cpp
===================================================================
--- Source/WebCore/html/shadow/SliderThumbElement.cpp	(revision 78999)
+++ Source/WebCore/html/shadow/SliderThumbElement.cpp	(working copy)
@@ -158,6 +158,12 @@
         return;
     }
 
+    HTMLInputElement* input = static_cast<HTMLInputElement*>(shadowHost());
+    if (input->isReadOnlyFormControl() || !input->isEnabledFormControl() ) {
+        HTMLDivElement::defaultEventHandler(event);
+        return;
+    }
+ 
     MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
     bool isLeftButton = mouseEvent->button() == LeftButton;
     const AtomicString& eventType = event->type();
Comment 3 Dimitri Glazkov (Google) 2011-02-23 12:32:12 PST
(In reply to comment #2)
> looks like this fixes it.  how does one write a layout test for such a thing ?
> 
> 
> Index: Source/WebCore/html/shadow/SliderThumbElement.cpp
> ===================================================================
> --- Source/WebCore/html/shadow/SliderThumbElement.cpp    (revision 78999)
> +++ Source/WebCore/html/shadow/SliderThumbElement.cpp    (working copy)
> @@ -158,6 +158,12 @@
>          return;
>      }
> 
> +    HTMLInputElement* input = static_cast<HTMLInputElement*>(shadowHost());
> +    if (input->isReadOnlyFormControl() || !input->isEnabledFormControl() ) {
> +        HTMLDivElement::defaultEventHandler(event);
> +        return;
> +    }
> + 
>      MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
>      bool isLeftButton = mouseEvent->button() == LeftButton;
>      const AtomicString& eventType = event->type();

There are lots of examples in existing LayoutTests. For example:

http://google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/fast/repaint/slider-thumb-drag-release.html&exact_package=chromium
Comment 4 Alexey Proskuryakov 2011-02-23 13:50:40 PST
Can anything be done to make mistakes like this less likely to occur elsewhere? It doesn't seem like it should be the job of every element that may end up being in a shadow tree to check if the host is disabled.
Comment 5 chris reiss 2011-02-23 14:39:06 PST
Let me try putting the mouse-event/shadow logic in the parent object, RenderBlock ...
Comment 6 Dimitri Glazkov (Google) 2011-02-23 14:42:45 PST
(In reply to comment #4)
> Can anything be done to make mistakes like this less likely to occur elsewhere? It doesn't seem like it should be the job of every element that may end up being in a shadow tree to check if the host is disabled.

This is a good question. Do shadow DOM subtrees of disabled hosts hear events? Hixie, WDYT?
Comment 7 Shinya Kawanaka 2011-07-23 06:12:15 PDT
Created attachment 101808 [details]
Patch
Comment 8 Alexey Proskuryakov 2011-07-23 10:41:59 PDT
> Can anything be done to make mistakes like this less likely to occur elsewhere? It doesn't seem like it should be the job of every element that may end up being in a shadow tree to check if the host is disabled.

This question still stands.

Also, what is the reason to add the checks in two places?
Comment 9 Shinya Kawanaka 2011-07-24 18:36:29 PDT
(In reply to comment #8)
> > Can anything be done to make mistakes like this less likely to occur elsewhere? It doesn't seem like it should be the job of every element that may end up being in a shadow tree to check if the host is disabled.
> 
> This question still stands.
> 
> Also, what is the reason to add the checks in two places?

They were necessary to make the slider work correctly on Mac. Just adding the code in RangeThumbSlider cannot disable the slider correctly on Mac, the other platform can though.

However, it is true that this patch does not answer the question. So I try to add some code in HTMLInputElement to disable shadow elements in all input type.
Comment 10 Kent Tamura 2011-07-24 18:43:41 PDT
(In reply to comment #8)
> Also, what is the reason to add the checks in two places?

A change for RangeInputType.cpp disables moving the thumb by clicking on the track not on the thumb, and a change for SliderThumbElement.cpp disables dragging the thumb.

Shinya, your test looks not to cover the former case.
Comment 11 Shinya Kawanaka 2011-07-26 01:08:23 PDT
Created attachment 101979 [details]
Patch
Comment 12 Shinya Kawanaka 2011-07-26 01:11:36 PDT
kent,

I've changed the test to cover the click on the knob and the click not on the knob.
However I omitted vertical slider version, because I don't think it is necessary to cover this scenario.

Alexey,

> Do shadow DOM subtrees of disabled hosts hear events? Hixie, WDYT?

This patch does not answer this question yet, however I prioritized fixing this regression bug.
Comment 13 Kent Tamura 2011-07-27 01:15:13 PDT
(In reply to comment #12)
> > Do shadow DOM subtrees of disabled hosts hear events? Hixie, WDYT?
> 
> This patch does not answer this question yet, however I prioritized fixing this regression bug.

I agree with Shinya.  We should discuss it thoroughly in another bug.
I don't think we should stop delivering events in such case.
Comment 14 WebKit Review Bot 2011-07-27 02:15:35 PDT
Comment on attachment 101979 [details]
Patch

Clearing flags on attachment: 101979

Committed r91827: <http://trac.webkit.org/changeset/91827>
Comment 15 WebKit Review Bot 2011-07-27 02:15:44 PDT
All reviewed patches have been landed.  Closing bug.