816340 - Propagate events to chrome even if there is a disabled form control in the event target chain

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

See also bug 804811. This will fix only the chrome part of bug 329509.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

No tests yet. Better to see how many existing tests this change breaks.
https://tbpl.mozilla.org/?tree=Try&rev=38cd39fe9917

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

https://tbpl.mozilla.org/?tree=Try&rev=cfd15dd0c101

Trying to keep the old drop handling.
We may want to change it, but not in this bug.

Comment 3

[[e-mail]]

Comment on attachment 687109 [details] [diff] [review]
patch

I made a build with this patch to see if I could see any effects.
1. Tooltips now appear on disabled form controls. This is a win.
2. Context menu broken for disabled text controls. This is unsurprising, as previously it didn't open at all. Ignoring text control context menu processing for disabled text controls seems to be the simplest workaround.
3. No effect observed in SeaMonkey Composer - still unable to double-click a disabled form control to open its properties. Might be a front-end bug?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

This fixes drop with less-hacky way. Takes care of also context menu on
disabled form controls.

https://tbpl.mozilla.org/?tree=Try&rev=68dbbfa64c83

Neil, feel free to comment.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3.1

https://tbpl.mozilla.org/?tree=Try&rev=cb40d821af54

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v4

Now with some tests

https://tbpl.mozilla.org/?tree=Try&rev=927433994b79

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Neil, could you provide some feedback?
After that I'll ask reviews from a DOM and from a browser peer.

Comment 8

[[e-mail]]

Comment on attachment 688405 [details] [diff] [review]
v4

Looks reasonable to me, in as much as it avoids changing behaviour (for instance I can see a use case for right-clicking on an <input type=image disabled> but that's beyond the scope of this bug).

>   canDropLink: function(aEvent, aAllowSameDocument)
>   {
>+    if (this._eventTargetIsDisabled(aEvent)) {
>+      return false;
>+    }
>     let dataTransfer = aEvent.dataTransfer;
>     let types = dataTransfer.types;
>     if (!types.contains("application/x-moz-file") &&
>         !types.contains("text/x-moz-url") &&
>         !types.contains("text/uri-list") &&
>         !types.contains("text/x-moz-text-internal") &&
>         !types.contains("text/plain"))
>       return false;
Nit: file style seems to be to avoid unnecessary {}s.

>-nsresult
>-nsHTMLTextAreaElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
>+bool
>+nsHTMLTextAreaElement::IsDisabledForEvents(uint32_t aMessage)
> {
>-  nsIFormControlFrame* formControlFrame = GetFormControlFrame(false);
>+nsIFormControlFrame* formControlFrame = GetFormControlFrame(false);
Oops ;-)
[I don't understand why IsElementDisabledForEvents can't call GetFormControlFrame itself]

Comment 9

[[e-mail]]

(In reply to comment #3)
> 1. Tooltips now appear on disabled form controls. This is a win.
Not true, because bug 274626 already fixed this. (And backing that out will prevent tooltips from working on disabled XUL textboxes, unless you tweak the patch somehow.)

> 3. No effect observed in SeaMonkey Composer - still unable to double-click a
> disabled form control to open its properties. Might be a front-end bug?
Indeed, editor.js adds its click handler to the content document body, so it still doesn't see the click. (Attaching the handler to the editor element helps but then the code needs to be tweaked to expect click events from intervening nodes.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

> [I don't understand why IsElementDisabledForEvents can't call
> GetFormControlFrame itself]
I want to keep the existing behavior. The change in this bug is major enough that regressions
are expected.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v5

https://tbpl.mozilla.org/?tree=Try&rev=4e8e7d3a081a

Jst, this removes the horrible b2g-only hack.
This is the first step to fix bug 329509. Better to cause first chrome regressions, and once those are fixed, change event handling in content too.
(But only after testing what other browsers do there. This stuff isn't
specified anywhere.)

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 688958 [details] [diff] [review]
v5

Looks good, sorry it took so long to get through this...

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 688958 [details] [diff] [review]
v5

Gavin, could you look at the browser bits?
For now I want to keep the existing behavior.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 688958 [details] [diff] [review]
v5

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>+  // Returns true if aNode is a from control (except text boxes and images),
>+  // or disabled for event handling.

We changed this code (and behavior) recently, see bug 433168. This patch doesn't apply on current trunk, and I think the trivial merge might also change behavior?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v6

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=4711cc833c6b

Comment 17

[:Gavin Sharp [email: [e-mail]]]

It would be helpful if there was a summary of under what conditions exactly IsElementDisabledForEvents(node) returns true - the comment in nsIDOMWindowUtils isn't particularly descriptive, and tracing through all the various implementations of IsDisabledForEvents gets a bit confusing. They all seem to delegate to nsGenericHTMLFormElement::IsElementDisabledForEvents, which I think means that IsElementDisabledForEvents returns true when:

- The element is a form control, AND
  - The element (or its fieldset) has the "disabled" attribute, OR
  - The element has the "-moz-user-input" style set to "none" or "disabled"

That's a very specific definition, maybe a more intuitive general one would be better.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 696524 [details] [diff] [review]
v6

>diff --git a/content/base/src/contentAreaDropListener.js b/content/base/src/contentAreaDropListener.js

>   dropLink: function(aEvent, aName, aDisallowInherit)
>   {
>     aName.value = "";
>+    if (this._eventTargetIsDisabled(aEvent))
>+      return null;

Callers of this function (particularly in browser.js) really don't expect it to return null. I think we probably need to fix the callers (to either always call canDropLink before, or handle a null return value).

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> It would be helpful if there was a summary of under what conditions exactly
> IsElementDisabledForEvents(node) returns true
The whole point is that it is a generic state, which happens to currently mean form controls
which are disabled using disabled attribute or -moz-user-input.
I could add documentation mentioning those as examples.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #18)
> Callers of this function (particularly in browser.js) really don't expect it
> to return null. I think we probably need to fix the callers (to either
> always call canDropLink before, or handle a null return value).
I could make it return "" since that is what the method may return already now.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #20)
> I could make it return "" since that is what the method may return already
> now.

That's an existing error state that's currently not handled very well by callers. But actually none of this really matters for this code, since the involved callers won't ever have a disabled form control as the target. So let's just ignore that issue and I can file a followup to clean up the existing code.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I'll change null to "" in any case. Better be consistent.

Would you like to see a new patch with that change and the added comment or could you review the
current patch?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v7

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/801ba75ac563