If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
See also bug 804811. This will fix only the chrome part of bug 329509.
(Assignee)

Comment 1

5 years ago
Created attachment 687109 [details] [diff] [review]
patch

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

Comment 2

5 years ago
Created attachment 687546 [details] [diff] [review]
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.
Attachment #687109 - Attachment is obsolete: true

Comment 3

5 years ago
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?
(Assignee)

Comment 4

5 years ago
Created attachment 687765 [details] [diff] [review]
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.
Attachment #687546 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 688323 [details] [diff] [review]
v3.1

https://tbpl.mozilla.org/?tree=Try&rev=cb40d821af54
Attachment #687765 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 688405 [details] [diff] [review]
v4

Now with some tests

https://tbpl.mozilla.org/?tree=Try&rev=927433994b79
Attachment #688323 - Attachment is obsolete: true
Attachment #688405 - Flags: feedback?(neil)
(Assignee)

Comment 7

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

Comment 8

5 years ago
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]
Attachment #688405 - Flags: feedback?(neil) → feedback+

Comment 9

5 years ago
(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.)
(Assignee)

Comment 10

5 years ago
> [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.
(Assignee)

Comment 11

5 years ago
Created attachment 688958 [details] [diff] [review]
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.)
Attachment #688405 - Attachment is obsolete: true
Attachment #688958 - Flags: review?(jst)
Comment on attachment 688958 [details] [diff] [review]
v5

Looks good, sorry it took so long to get through this...
Attachment #688958 - Flags: review?(jst) → review+
(Assignee)

Comment 13

5 years ago
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.
Attachment #688958 - Flags: review?(gavin.sharp)
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?
Attachment #688958 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 15

5 years ago
Created attachment 696524 [details] [diff] [review]
v6
Attachment #696524 - Flags: review?(gavin.sharp)
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4711cc833c6b
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 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).
(Assignee)

Comment 19

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(Assignee)

Comment 20

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(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.
(Assignee)

Comment 22

5 years ago
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?
Attachment #688958 - Attachment is obsolete: true
Attachment #696524 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 697448 [details] [diff] [review]
v7
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/mozilla-central/rev/801ba75ac563
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 827017

Updated

5 years ago
Blocks: 827018

Updated

3 years ago
Depends on: 1140725
You need to log in before you can comment on or make changes to this bug.