375008 - clicking disabled button should not cause focus change

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[mcdavis941 (sporadically reading bugmail)]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

Per recommendation from Neil (discussion at http://groups.google.com/group/mozilla.dev.extensions/browse_thread/thread/c527eff8cd746112/d7359ba7cf38ae01?#d7359ba7cf38ae01), reporting this as a bug:

When you click a disabled button/checkbox when some other element
is focused, the other element loses focus.  

1. On the Options window "Main" panel, set up the checkboxes under the
Downloads groupbox like this:

Show the Downloads window when downloading a file -- checked
Close it when all downloads are finished -- checked

2. Now uncheck "Show the Downloads window" so that "Close it when all
downloads are finished" becomes disabled.

3. Tab to the Cancel button so it has focus.

4. Click the now-disabled "Close it when all
downloads are finished"

5. Cancel button loses focus

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1.1) Gecko/20061204
Firefox/2.0.0.1
Using default theme. 

I first saw this with 2.0.0.1, but I'm seeing it now with 2.0.0.3.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[:Gijs (he/him)]

This is marked as a regression, do we know when this regressed? Did it work in 1.5, for example?

Comment 2

[[e-mail]]

Not a very good regression range I know, but Netscape 7.2 doesn't have the bug, while a 1.8 branch build I made on 16th December 2005 does have the bug.

Comment 3

[[e-mail]]

It's not much help but I found a build from 13th November 2005 had the bug.

Comment 4

[:Gijs (he/him)]

Reproducible on Fx 0.9+ from 2004-07-26-14, but not on Fx 0.9+ from 2004-07-24-09. If this is x-platform it could be narrowed down a little bit more, unfortunately there are no builds from the 25th for win32.

Regression range as is: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-07-24+08%3A00%3A00&maxdate=2004-07-26+14%3A00%3A00&cvsroot=%2Fcvsroot

I suspect bug 250006 rather heavily, though I haven't looked through the entire list, this one looks very likely to have caused this breakage.

Comment 5

[:Gijs (he/him)]

OK, so this is where (I think) things go wrong. Someone please tell me whether I'm making sense:

In nsEventStateManager::PostHandleEvent (http://mxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#2275)
we decide we won't suppress blurs
(http://mxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#2325)
and we loop over the parent chain of curFrame, and none of these items are focusable, but that means we eventually fire

SetContentState(nsnull, NS_EVENT_STATE_FOCUS);
(http://mxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#2356)

Which then sends a blur event to the currently focused thing and removes the focus ring. I'm not sure where to fix this, though. :-\

I do have a debug build so if someone wants to help me figure this out, that'd be grand. I'm available most of this weekend, CET.

Comment 6

[:Gijs (he/him)]

Mats, Aaron told me you're the resident focus guru. :-)
Any chance you could take a look at this still?

Comment 7

[Marco Zehe (:MarcoZ)]

This is still valid for Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9) Gecko/2008052906 Firefox/3.0

Comment 8

[Adam]

Attachment: Adds -moz-user-focus: ignore style in the appropriate theme stylesheets.

Style-only change, which uses the fact that if the clicked-on element is -moz-user-focus: ignore, then it won't affect the focus.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v1 - Fix the bug for html content

This patch might not fix the bug for XUL elements but will certainly fix it for HTML elements. We should open a new bug for XUL elements.

Comment 10

[Neil Deakin]

Comment on attachment 615697 [details] [diff] [review]
Patch v1 - Fix the bug for html content

>+          if (!suppressBlur) {
>+            nsCOMPtr<Element> element = do_QueryInterface(aEvent->target);

Are you using aEvent->target because it refers to the element, whereas activeContent might be a text node inside it?

> We should open a new bug for XUL elements.

I don't think there is a bug is there? XUL elements handle -moz-user-focus and should work fine, no?

I gave up on reviewing the test because it's completely unreadable. It would be readable if you just used markup for the elements and didn't have this odd hitEventLoop function that doesn't explain what you're waiting for.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Neil Deakin from comment #10)
> Comment on attachment 615697 [details] [diff] [review]
> Patch v1 - Fix the bug for html content
> 
> >+          if (!suppressBlur) {
> >+            nsCOMPtr<Element> element = do_QueryInterface(aEvent->target);
> 
> Are you using aEvent->target because it refers to the element, whereas
> activeContent might be a text node inside it?

Yes.

> > We should open a new bug for XUL elements.
> 
> I don't think there is a bug is there? XUL elements handle -moz-user-focus
> and should work fine, no?

Like HTML elements. My concern is that some elements in Firefox chrome steal focus when they are disabled but don't get focused (because they are disabled). Exactly like <button> was behaving.
It might be a Firefox bug. I don't know much about XUL.

> I gave up on reviewing the test because it's completely unreadable. It would
> be readable if you just used markup for the elements and didn't have this
> odd hitEventLoop function that doesn't explain what you're waiting for.

hitEventLoop is because when the focus doesn't happen we can't actually check that so we have to hit a few times the event to assume the focus didn't really happen. I put 5 but 1 or 2 might be enough actually. I could check that.
If you want me to make the test more readable I would be happy to. I'm unfortunately used to write tests that nobody read... :(

Comment 12

[Neil Deakin]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11)
> Like HTML elements. My concern is that some elements in Firefox chrome steal
> focus when they are disabled but don't get focused (because they are
> disabled). Exactly like <button> was behaving.
> It might be a Firefox bug. I don't know much about XUL.

Are you seeing a specific bug?

Disabled handling for xul elements should happen at http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#571

> hitEventLoop is because when the focus doesn't happen we can't actually
> check that so we have to hit a few times the event to assume the focus
> didn't really happen.

Since focusing an element with the mouse isn't asynchronous, you should be able to check the current focus right after synthesizeMouseAtCenter is called. You could also use synthesizeMouseExpectEvent.

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to Neil Deakin from comment #12)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11)
> > Like HTML elements. My concern is that some elements in Firefox chrome steal
> > focus when they are disabled but don't get focused (because they are
> > disabled). Exactly like <button> was behaving.
> > It might be a Firefox bug. I don't know much about XUL.
> 
> Are you seeing a specific bug?
> 
> Disabled handling for xul elements should happen at
> http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/
> nsXULElement.cpp#571

I can't bet on that but I don't think ::IsFocusable() is involved in this bug. Button are not focusable but they were not igoring the click which means they were making the previous element to blur but were still not focused.

> > hitEventLoop is because when the focus doesn't happen we can't actually
> > check that so we have to hit a few times the event to assume the focus
> > didn't really happen.
> 
> Since focusing an element with the mouse isn't asynchronous, you should be
> able to check the current focus right after synthesizeMouseAtCenter is
> called. You could also use synthesizeMouseExpectEvent.

I was actually wondering if that was the case. I will update the test and re-attach it.

Comment 14

[Neil Deakin]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #13)
> I can't bet on that but I don't think ::IsFocusable() is involved in this
> bug. Button are not focusable but they were not igoring the click which
> means they were making the previous element to blur but were still not
> focused.

OK, I see what you are referring to. That is a bug, perhaps a regression.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Patch v1 - Fix the bug for html content

Comment 16

[Adam]

Trying on http://www.javascriptkit.com/javatutors/deform2.shtml I cannot reproduce this for HTML elements on Linux (that page has disabled input:submit, input:text, and select).  Is this a bug for HTML forms on other platforms?  (The style "-moz-user-focus:ignore;" removed by the patch is what prevents the blur for HTML form elements.)

Note the original submission discussed this in the context of chrome elements, and I can exhibit the problem there on Linux (in the preferences as the STR mentions).

Anyway, to fix this for chrome with the grain of the new patch, repeat the "if (!suppressBlur)" check, but for XUL Controls:

if (!suppressBlur) {
  nsCOMPtr<nsIDOMXULControlElement> xulControl = do_QueryObject(
    aEvent->target);
  if (xulControl) {
    xulControl->GetDisabled(&suppressBlur);
  }
}

Comment 17

[Mounir Lamouri (:mounir)]

(In reply to Adam [:hobophobe] from comment #16)
> Trying on http://www.javascriptkit.com/javatutors/deform2.shtml I cannot
> reproduce this for HTML elements on Linux (that page has disabled
> input:submit, input:text, and select).

Have you tried <button>?

> Is this a bug for HTML forms on
> other platforms?  (The style "-moz-user-focus:ignore;" removed by the patch
> is what prevents the blur for HTML form elements.)

I know but that hack is ugly.

> Note the original submission discussed this in the context of chrome
> elements, and I can exhibit the problem there on Linux (in the preferences
> as the STR mentions).
> 
> Anyway, to fix this for chrome with the grain of the new patch, repeat the
> "if (!suppressBlur)" check, but for XUL Controls:
> 
> if (!suppressBlur) {
>   nsCOMPtr<nsIDOMXULControlElement> xulControl = do_QueryObject(
>     aEvent->target);
>   if (xulControl) {
>     xulControl->GetDisabled(&suppressBlur);
>   }
> }

Indeed. I did that but I would like to try to use nsIContent::IsFocusable(). That *should* work but it creates a regression I would like to understand. If it seems too hard I will open a follow-up and simply attach a patch checking for disabled state of XUL and HTML elements. But I don't know when, I have hardware issues right now...

Comment 18

[Neil Deakin]

The right fix here for XUL elements is to either change IsFocusable to be able to optionally override the suppressBlur value, or to add -moz-user-focus: ignore to the various disabled controls.

Implementing the former could also work for other types of elements.

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to Neil Deakin from comment #18)
> The right fix here for XUL elements is to either change IsFocusable to be
> able to optionally override the suppressBlur value

If IsFocusable() returns false, suppressBlur should be true. Unfortunately, doing that prevents focusing <body> by clicking on it.

> or to add -moz-user-focus: ignore to the various disabled controls.

So, this is why <button> had a bug and this is error prone. Though, it could be done cleverly maybe. However, we already have the focus logic in IsFocusable() so I think it would be better to keep everything here. For example, some elements might be unfocusable for other reasons than being disabled.

Comment 20

[Neil Deakin]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #19)
> If IsFocusable() returns false, suppressBlur should be true.

That's not correct. It should continue looking up ancestors without changing suppressBlur.

supressBlur should only be true when -moz-user-focus: ignore is used, or as this bug proposes, specific other cases.

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to Neil Deakin from comment #20)
> That's not correct. It should continue looking up ancestors without changing
> suppressBlur.

Out of curiosity, is this per spec? Or are UA allowed to do whatever they want?

Comment 22

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Fix HTML and XUL elements.

Comment 23

[Neil Deakin]

Comment on attachment 616104 [details] [diff] [review]
Patch v2

While the changes to nsEventStateManager look ok, I'm not sure if I like hardcoding specific cases where suppressBlur should be used here, rather than in IsFocusable, or by using -moz-user-focus.

I was also hoping you'd make the test use markup instead.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

We can't IsFocusable() because we still want to blur if we try to focus a <div> or the background. I added a comment regarding this.

Also, I cleaned up the tests.

Comment 25

[Neil Deakin]

Comment on attachment 712510 [details] [diff] [review]
Patch v2.1

>+          activeContent = mCurrentTarget->GetContent();
>+
>+          // In some cases, we do not want to even blur the current focused
>+          // element. Those cases are:
>+          // 1. -moz-user-focus CSS property is set to 'ignore';
>+          // 2. HTML element has :disabled pseudo-class applying;

This actually applies to any element, not just HTML elements. Technically, it also applies when the element has the disabled event state, rather than specifically the pseudo-class.

>+          // 3. XUL control element has the disabled attribute set to 'true.

attribute -> property

>+
>+  <!-- Hidden elements, they have a frame but focus will go trough them. -->

trough -> through

>+
>+    // Cleanup.
>+//    element.hidden = true;

Remove the commented out line

>+    witness.focus();
>+  }
>+
>+  var focusableElements = document.getElementById('focusable').children;
>+  for (var i=0; i<focusableElements.length; ++i) {
>+    var element = focusableElements[i];
>+    element.hidden = false;
>+    synthesizeMouseAtCenter(element, {});
>+    is(document.activeElement, element, "focus should have move to " + element);

'have move to' -> 'have moved to'

Comment 26

[Mounir Lamouri (:mounir)]

Thanks for the review Neil :)
I applied your review comments in the pushed patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c59b73c8c4b

Comment 27

[Mounir Lamouri (:mounir)]

Landed again with the tests hopefully fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18266cf1e463

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18266cf1e463