Last Comment Bug 426082 - Hovering on label should indicate for which checkbox/radiobutton belongs to
: Hovering on label should indicate for which checkbox/radiobutton belongs to
Status: VERIFIED FIXED
[parity-IE][parity-webkit][polish-int...
: polish, pp, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- enhancement with 5 votes (vote)
: mozilla1.9.3a5
Assigned To: Markus Stange [:mstange]
:
: Jet Villegas (:jet)
Mentors:
Depends on: 562932 626168
Blocks: 164421 656379
  Show dependency treegraph
 
Reported: 2008-03-30 11:14 PDT by Emil Ivanov
Modified: 2011-05-11 11:01 PDT (History)
17 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2-
mstange: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (15.72 KB, image/png)
2008-03-30 12:34 PDT, Emil Ivanov
no flags Details
testcase (356 bytes, text/html)
2010-03-09 16:42 PST, Emil Ivanov
no flags Details
:hover / :active testcase (689 bytes, text/html)
2010-04-27 04:34 PDT, Markus Stange [:mstange]
no flags Details
:hover / :active testcase (685 bytes, text/html)
2010-04-27 04:35 PDT, Markus Stange [:mstange]
no flags Details
v1 (23.16 KB, patch)
2010-04-27 04:41 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
v2 (29.63 KB, patch)
2010-04-27 10:38 PDT, Markus Stange [:mstange]
dbaron: review-
Details | Diff | Splinter Review
part 1, v3 (4.65 KB, patch)
2010-04-30 04:21 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v3 (9.15 KB, patch)
2010-04-30 04:21 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 3, v3 (8.34 KB, patch)
2010-04-30 04:22 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 4, v3 (8.11 KB, patch)
2010-04-30 04:22 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 5, v3 (5.51 KB, patch)
2010-04-30 04:22 PDT, Markus Stange [:mstange]
bugs: review+
Details | Diff | Splinter Review
part 4, v4 (7.49 KB, patch)
2010-05-18 12:21 PDT, Markus Stange [:mstange]
bugs: review+
Details | Diff | Splinter Review

Description Emil Ivanov 2008-03-30 11:14:28 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008032904 Minefield/3.0pre
Build Identifier: 

This is implemented in firefox interface, but i cant see it inside html pages.

Reproducible: Always
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2008-03-30 11:35:17 PDT
I don't see anything like this "in firefox interface"...  What am I missing?

This bug needs a clear list of expected and actual behavior.  Attach screenshots if needed to explain what you want.
Comment 2 Emil Ivanov 2008-03-30 12:00:10 PDT
Sorry i though the title is saying enough.

1. Open Customize Toolbar
2. Hover on the text "use small icons"
3. Go to the bottom of this page and hover on "resolve bug" 

Actual results:
Nothing happens when point on labels in html pages.

Expected:
When point on label the checkbox for that label should change state
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2008-03-30 12:13:39 PDT
I don't see the behavior you describe on Mac, for what it's worth...  The reason it might be happening in XUL is that the checkbox+label is a single element that enters hover state.  That's not the case in HTML: they're two separate elements.  Putting the checkbox into :hover when it's not actually hovered is likely to confuse web pages somewhat...
Comment 4 Emil Ivanov 2008-03-30 12:34:55 PDT
Created attachment 312636 [details]
screenshot

OK then could this belong for OS integration, since IE does that on pages and everywhere in windows is like that.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2008-03-30 13:32:01 PDT
Does the checkbox actually go into :hover in IE?  That is, if you have :hover styles applied to it, are they triggered?  Does IE even support :hover on checkboxes?
Comment 6 Emil Ivanov 2008-03-30 14:44:02 PDT
No this is native behavior, thats why it is present only in IE and windows programs, except pages displayed in non IE browsers. If the checkbox is styled from the page it will loose the native theming.

With hover i mean when pointing over label, not the :hover rule.

On windows if you are able to select the checkbox when you click on the label, then if you only point over the label the checkbox for that label should change state.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2008-03-30 15:09:56 PDT
So pointing at the checkbox does NOT make it change state?
Comment 8 Emil Ivanov 2008-03-30 23:03:43 PDT
When pointing on checkbox it changes state, as pointing on label. But only in IE. In firefox there is no visual effect when pointing only on the label.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2008-03-30 23:46:55 PDT
OK.  Confirming, but someone on Windows will need to pick this up, I guess...
Comment 10 Marat Tanalin | tanalin.com 2008-12-02 08:10:53 PST
Besides, now correct behavior of radio buttons / checkboxes (radio button / checkbox is hilited when its label is hovered just like when radio button / checkbox itself is hovered) is typical not only for IE, but also for Google Chrome (Windows).

It would be nice to see such feature in upcoming Firefox 3.1. In addition to *usability* improvement, it means even more seamless *OS integration* that was one of a major goals for Firefox 3. Thanks.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2008-12-02 09:38:17 PST
Feature freeze for Firefox 3.1 (and this is definitely a feature) was months ago.
Comment 12 Emil Ivanov 2009-02-28 01:22:06 PST
Safari 4 Beta go this too.
Comment 13 u49640 2009-03-18 12:18:14 PDT
(In reply to comment #12)
> Safari 4 Beta go this too.

Appearently not on a Mac
Comment 14 Marat Tanalin | tanalin.com 2009-03-18 13:51:48 PDT
(In reply to comment #13)
Matthias Wallner:
Such behavior is native for *Windows*, not for Mac OS (see the Platform field of the bug).
Comment 15 u49640 2009-03-19 09:12:06 PDT
(In reply to comment #14)
> (In reply to comment #13)
> Matthias Wallner:
> Such behavior is native for *Windows*, not for Mac OS (see the Platform field
> of the bug).

Thanks.

The platform field is never accurate in BugZilla, since most cross-platform bugs are filed "Windows" Bugs, so i mentioned it.
Comment 16 Worcester12345 2009-09-11 11:57:15 PDT
Can this be blocking  Bug 251198 -  Bright, clear focus appearance for HTML content on Win/GTK platforms?
Comment 17 Emil Ivanov 2010-03-09 16:42:48 PST
Created attachment 431503 [details]
testcase
Comment 18 Jose Fandos 2010-03-09 17:46:40 PST
Someone with the rights to enter details in the Whiteboard and/or Keywords fields could add one to bring this to the attention of Alex Faarborg. It's not for parity with other browsers, or not only, it's more to follow Windows UI guidelines.
Comment 19 Markus Stange [:mstange] 2010-04-27 04:34:15 PDT
Created attachment 441766 [details]
:hover / :active testcase

In this testcase you can see that IE doesn't make :hover match on the control, but Webkit does. However, Webkit doesn't set the hover state on the control's ancestor elements, contrary to what the :hover state usually does.

I propose we go the IE route and only return NS_EVENT_STATE_HOVER / _ACTIVE for labeled controls when GetContentState is called from native theming, and not in the usual :hover/:active selector matching case.
Comment 20 Markus Stange [:mstange] 2010-04-27 04:35:46 PDT
Created attachment 441768 [details]
:hover / :active testcase

This is the testcase I wanted to upload.
Comment 21 Markus Stange [:mstange] 2010-04-27 04:41:48 PDT
Created attachment 441770 [details] [diff] [review]
v1

This is a combined diff of the four parts on
http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/pushloghtml?changeset=da2e7995a9c9

Is the static_cast to nsHTMLLabelElement* acceptable? I've seen this being done with nsHTMLMediaElement, for example, but in many other similar cases new interfaces like nsINS... are introduced, so I'm not sure which way to go here.
Comment 22 Olli Pettay [:smaug] 2010-04-27 05:11:55 PDT
static_cast is ok in that case, IMO, since you know the element is really
a natively implemented (nsIContent).

Have you tested things like removing label from document when it is being hovered/activated (what happens to the related form control)?

This really needs tests. And dbaron should probably review :hover/:active/etc
handling changes.
Comment 23 Markus Stange [:mstange] 2010-04-27 10:38:37 PDT
Created attachment 441847 [details] [diff] [review]
v2

http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/pushloghtml?changeset=e7fdbbd9b2c8

(In reply to comment #22)
> static_cast is ok in that case, IMO, since you know the element is really
> a natively implemented (nsIContent).

Ah, ok.

> Have you tested things like removing label from document when it is being
> hovered/activated (what happens to the related form control)?

This was indeed broken because nsEventStateManager::ContentRemoved set mHover/ActiveContent directly. I've made it use SetContentState instead. This does some additional work (like notifying to-be-removed elements), which might or might not make a difference...

> This really needs tests.

Added some. It was a little tricky because it's only observable visually, so I had to use drawWindow and MozAfterPaint.
Comment 24 David Baron :dbaron: ⌚️UTC-10 2010-04-28 15:09:55 PDT
Could you describe the changes you're making?  What are the expected behavior changes in terms of selector matching and in terms of UI behavior?
Comment 25 Markus Stange [:mstange] 2010-04-28 22:25:41 PDT
They're described in the commit messages on http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/pushloghtml?changeset=e7fdbbd9b2c8
Comment 26 David Baron :dbaron: ⌚️UTC-10 2010-04-29 14:27:37 PDT
This looks like it would be much easier to review as those 5 separate patches (and with those descriptions, too) rather than as 1 big patch.  Any reason you combined them for review?
Comment 27 David Baron :dbaron: ⌚️UTC-10 2010-04-29 14:43:48 PDT
Comment on attachment 441847 [details] [diff] [review]
v2

review denied on this mega-patch that mixes everything together and is
hard to follow.

However:


http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/rev/794ae6414237

>+static PRBool
>+IsAncestorOf(nsIContent* aTarget, nsIContent* aStartNode)

Should be |static bool| given the values you're returning.

I'd also prefer if you use the same parameter names (except backwards)
as nsContentUtils::ContentIsDescendantOf.  I think those names are clearer.

r=dbaron on that patch


http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/rev/a2ead99e6891

r=dbaron on that patch

http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/rev/a11ef76eff5e

r=dbaron on that patch, although you need a content peer to review too

http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/rev/96eb547d6091

>+static already_AddRefed<nsIContent>
>+GetLabelTarget(nsIContent* aLabel)
>+{
>+  nsCOMPtr<nsIDOMHTMLLabelElement> label = do_QueryInterface(aLabel);

This QI might turn out to be a performance problem.

smaug would probably be better at suggesting alternatives, though.

r=dbaron on this patch too.
Comment 28 Markus Stange [:mstange] 2010-04-30 03:44:06 PDT
(In reply to comment #26)
> This looks like it would be much easier to review as those 5 separate patches
> (and with those descriptions, too) rather than as 1 big patch.  Any reason you
> combined them for review?

It was an experiment. I was looking for a way to avoid attaching all the patch parts one-by-one to the bug and re-attaching them after every review. Pushing the patches to a repo (tryserver-style) and just attaching the result of hg diff -r qparent:qtip seemed like a simple solution, and it even lets reviewers see what the affected files look like between individual patch parts.
But now I think those advantages are outweighed by the loss of per-part-review-state trackability in bugzilla and by complicating the reviewer's task. Another thing I hadn't considered is that hgweb only provides three lines of patch context.
Comment 29 Markus Stange [:mstange] 2010-04-30 04:21:25 PDT
Created attachment 442658 [details] [diff] [review]
part 1, v3
Comment 30 Markus Stange [:mstange] 2010-04-30 04:21:52 PDT
Created attachment 442659 [details] [diff] [review]
part 2, v3
Comment 31 Markus Stange [:mstange] 2010-04-30 04:22:11 PDT
Created attachment 442660 [details] [diff] [review]
part 3, v3
Comment 32 Markus Stange [:mstange] 2010-04-30 04:22:37 PDT
Created attachment 442661 [details] [diff] [review]
part 4, v3
Comment 33 Markus Stange [:mstange] 2010-04-30 04:22:56 PDT
Created attachment 442662 [details] [diff] [review]
part 5, v3
Comment 34 Markus Stange [:mstange] 2010-05-01 01:37:12 PDT
Comment on attachment 442660 [details] [diff] [review]
part 3, v3

I think this will become obsolete with bug 562932.
Comment 35 Markus Stange [:mstange] 2010-05-01 01:37:43 PDT
Comment on attachment 442661 [details] [diff] [review]
part 4, v3

I'll have to update this on top of bug 562932, which adds nsIDOMNSLabelElement.
Comment 36 Mounir Lamouri (:mounir) 2010-05-01 05:34:28 PDT
Indeed, I think it will be easier to remove part 3 and use |GetControl| in part 4. |GetControl| is returning a nsIDOMHTMLElement so you will have to QI to nsIContent (static_cast could be used here maybe).
Comment 37 Olli Pettay [:smaug] 2010-05-03 03:25:10 PDT
I guess this should be review after bug 562932.
Comment 38 Markus Stange [:mstange] 2010-05-18 12:21:24 PDT
Created attachment 446016 [details] [diff] [review]
part 4, v4

on top of bug 562932
Comment 39 Olli Pettay [:smaug] 2010-05-25 08:30:31 PDT
Comment on attachment 446016 [details] [diff] [review]
part 4, v4

>diff --git a/content/events/public/nsIEventStateManager.h b/content/events/public/nsIEventStateManager.h
>--- a/content/events/public/nsIEventStateManager.h
>+++ b/content/events/public/nsIEventStateManager.h
>@@ -80,17 +80,18 @@ public:
>                              nsIView* aView) = 0;
> 
>   NS_IMETHOD SetPresContext(nsPresContext* aPresContext) = 0;
>   NS_IMETHOD ClearFrameRefs(nsIFrame* aFrame) = 0;
> 
>   NS_IMETHOD GetEventTarget(nsIFrame **aFrame) = 0;
>   NS_IMETHOD GetEventTargetContent(nsEvent* aEvent, nsIContent** aContent) = 0;
> 
>-  virtual PRInt32 GetContentState(nsIContent *aContent) = 0;
>+  virtual PRInt32 GetContentState(nsIContent *aContent,
>+                                  PRBool aFollowLabels = PR_FALSE) = 0;
Could you document what aFollowLabels means.

>+static already_AddRefed<nsIContent>
>+GetLabelTarget(nsIContent* aLabel)
>+{
>+  nsCOMPtr<nsIDOMNSHTMLLabelElement> label = do_QueryInterface(aLabel);
>+  if (!label)
>+    return nsnull;
>+
>+  nsCOMPtr<nsIDOMHTMLElement> target;
>+  label->GetControl(getter_AddRefs(target));
>+  nsIContent* targetContent = nsnull;
>+  if (target) {
>+    CallQueryInterface(target, &targetContent);
>+  }
>+  return targetContent;
>+}
>+
> static bool
>-IsAncestorOf(nsIContent* aPossibleAncestor, nsIContent* aPossibleDescendant)
>+IsAncestorOf(nsIContent* aPossibleAncestor, nsIContent* aPossibleDescendant,
>+             PRBool aFollowLabels)
> {
>   for (; aPossibleDescendant; aPossibleDescendant = aPossibleDescendant->GetParent()) {
>     if (aPossibleAncestor == aPossibleDescendant)
>       return true;
>+
>+    if (aFollowLabels) {
>+      nsCOMPtr<nsIContent> labelTarget = GetLabelTarget(aPossibleDescendant);
>+      if (labelTarget == aPossibleAncestor)
>+        return true;
>+    }
>   }
>   return false;
> }
> 
> PRInt32
>-nsEventStateManager::GetContentState(nsIContent *aContent)
>+nsEventStateManager::GetContentState(nsIContent *aContent, PRBool aFollowLabels)
> {
>   PRInt32 state = aContent->IntrinsicState();
> 
>-  if (IsAncestorOf(aContent, mActiveContent)) {
>+  if (IsAncestorOf(aContent, mActiveContent, aFollowLabels)) {
>     state |= NS_EVENT_STATE_ACTIVE;
>   }
>-  if (IsAncestorOf(aContent, mHoverContent)) {
>+  if (IsAncestorOf(aContent, mHoverContent, aFollowLabels)) {
>     state |= NS_EVENT_STATE_HOVER;
>   }
> 
>   nsFocusManager* fm = nsFocusManager::GetFocusManager();
>   nsIContent* focusedContent = fm ? fm->GetFocusedContent() : nsnull;
>   if (aContent == focusedContent) {
>     state |= NS_EVENT_STATE_FOCUS;
> 
>@@ -4066,16 +4090,20 @@ static nsIContent* FindCommonAncestor(ns
> }
> 
> static void
> NotifyAncestors(nsIDocument* aDocument, nsIContent* aStartNode,
>                 nsIContent* aStopBefore, PRInt32 aState)
> {
>   while (aStartNode && aStartNode != aStopBefore) {
>     aDocument->ContentStatesChanged(aStartNode, nsnull, aState);
>+    nsCOMPtr<nsIContent> labelTarget = GetLabelTarget(aStartNode);
>+    if (labelTarget) {
>+      aDocument->ContentStatesChanged(labelTarget, nsnull, aState);
>+    }
>     aStartNode = aStartNode->GetParent();
>   }
> }
> 
> PRBool
> nsEventStateManager::SetContentState(nsIContent *aContent, PRInt32 aState)
> {
>   const PRInt32 maxNotify = 5;
>@@ -4276,24 +4304,24 @@ nsEventStateManager::ContentRemoved(nsID
>   nsFocusManager* fm = nsFocusManager::GetFocusManager();
>   if (fm)
>     fm->ContentRemoved(aDocument, aContent);
> 
>   if (mHoverContent &&
>       nsContentUtils::ContentIsDescendantOf(mHoverContent, aContent)) {
>     // Since hover is hierarchical, set the current hover to the
>     // content's parent node.
>-    mHoverContent = aContent->GetParent();
>+    SetContentState(aContent->GetParent(), NS_EVENT_STATE_HOVER);
>   }
> 
>   if (mActiveContent &&
>       nsContentUtils::ContentIsDescendantOf(mActiveContent, aContent)) {
>     // Active is hierarchical, so set the current active to the
>     // content's parent node.
>-    mActiveContent = aContent->GetParent();
>+    SetContentState(aContent->GetParent(), NS_EVENT_STATE_ACTIVE);
>   }
> 
>   if (mDragOverContent &&
>       nsContentUtils::ContentIsDescendantOf(mDragOverContent, aContent)) {
>     mDragOverContent = nsnull;
>   }
> 
>   if (mLastMouseOverElement &&
>diff --git a/content/events/src/nsEventStateManager.h b/content/events/src/nsEventStateManager.h
>--- a/content/events/src/nsEventStateManager.h
>+++ b/content/events/src/nsEventStateManager.h
>@@ -115,17 +115,18 @@ public:
> 
>   NS_IMETHOD NotifyDestroyPresContext(nsPresContext* aPresContext);
>   NS_IMETHOD SetPresContext(nsPresContext* aPresContext);
>   NS_IMETHOD ClearFrameRefs(nsIFrame* aFrame);
> 
>   NS_IMETHOD GetEventTarget(nsIFrame **aFrame);
>   NS_IMETHOD GetEventTargetContent(nsEvent* aEvent, nsIContent** aContent);
> 
>-  virtual PRInt32 GetContentState(nsIContent *aContent);
>+  virtual PRInt32 GetContentState(nsIContent *aContent,
>+                                  PRBool aFollowLabels = PR_FALSE);
>   virtual PRBool SetContentState(nsIContent *aContent, PRInt32 aState);
>   NS_IMETHOD ContentRemoved(nsIDocument* aDocument, nsIContent* aContent);
>   NS_IMETHOD EventStatusOK(nsGUIEvent* aEvent, PRBool *aOK);
> 
>   // Access Key Registration
>   NS_IMETHOD RegisterAccessKey(nsIContent* aContent, PRUint32 aKey);
>   NS_IMETHOD UnregisterAccessKey(nsIContent* aContent, PRUint32 aKey);
>   NS_IMETHOD GetRegisteredAccessKey(nsIContent* aContent, PRUint32* aKey);
>diff --git a/widget/src/xpwidgets/nsNativeTheme.cpp b/widget/src/xpwidgets/nsNativeTheme.cpp
>--- a/widget/src/xpwidgets/nsNativeTheme.cpp
>+++ b/widget/src/xpwidgets/nsNativeTheme.cpp
>@@ -85,17 +85,17 @@ nsNativeTheme::GetContentState(nsIFrame*
>   if (!aFrame->GetContent())
>     return 0;
> 
>   nsIPresShell *shell = GetPresShell(aFrame);
>   if (!shell)
>     return 0;
> 
>   nsIEventStateManager* esm = shell->GetPresContext()->EventStateManager();
>-  PRInt32 flags = esm->GetContentState(aFrame->GetContent());
>+  PRInt32 flags = esm->GetContentState(aFrame->GetContent(), PR_TRUE);
>   
>   if (isXULCheckboxRadio && aWidgetType == NS_THEME_RADIO) {
>     if (IsFocused(aFrame))
>       flags |= NS_EVENT_STATE_FOCUS;
>   }
> 
>   // On Windows and Mac, only draw focus rings if they should be shown. This
>   // means that focus rings are only shown once the keyboard has been used to
Comment 41 Markus Stange [:mstange] 2010-05-31 12:12:57 PDT
Test fixup:
http://hg.mozilla.org/mozilla-central/rev/3ba26104a2f4
Comment 42 Emil Ivanov 2010-06-01 12:57:34 PDT
Verified with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100601 Minefield/3.7a5pre

Great the behavior is exactly like in Windows, its even better than IE which doesnt trigger active state on the input when clicking on label.

I filled this bug for Windows platform, but now its for All.

Does that means this will be valid for all other platforms?
Comment 43 Markus Stange [:mstange] 2010-06-01 13:02:50 PDT
(In reply to comment #42)
> I filled this bug for Windows platform, but now its for All.
> 
> Does that means this will be valid for all other platforms?

Yes, to the extent that it applies to them.
For example, native controls on the Mac don't have hover feedback, but they have "pressed" feedback (they become a little darker); now pressing the label will make them look pressed, too.

Note You need to log in before you can comment on or make changes to this bug.