Closed Bug 426082 Opened 13 years ago Closed 11 years ago

Hovering on label should indicate for which checkbox/radiobutton belongs to


(Core :: Layout: Form Controls, enhancement)

Not set





(Reporter: stream, Assigned: mstange)



(Keywords: platform-parity, polish, testcase, Whiteboard: [parity-IE][parity-webkit][polish-interactive][polish-p2])


(7 files, 6 obsolete files)

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
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.
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.

When point on label the checkbox for that label should change state
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...
Attached image screenshot
OK then could this belong for OS integration, since IE does that on pages and everywhere in windows is like that.
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?
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.
So pointing at the checkbox does NOT make it change state?
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.
OK.  Confirming, but someone on Windows will need to pick this up, I guess...
Ever confirmed: true
Version: unspecified → Trunk
Blocks: 164421
Severity: major → enhancement
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.
Feature freeze for Firefox 3.1 (and this is definitely a feature) was months ago.
Whiteboard: [parity-IE] [parity-chrome]
Flags: blocking1.9.2?
Whiteboard: [parity-IE] [parity-chrome] → [parity-IE] [parity-Chrome] [parity-Safari]
Safari 4 Beta go this too.
(In reply to comment #12)
> Safari 4 Beta go this too.

Appearently not on a Mac
(In reply to comment #13)
Matthias Wallner:
Such behavior is native for *Windows*, not for Mac OS (see the Platform field of the bug).
(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).


The platform field is never accurate in BugZilla, since most cross-platform bugs are filed "Windows" Bugs, so i mentioned it.
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Can this be blocking  Bug 251198 -  Bright, clear focus appearance for HTML content on Win/GTK platforms?
Attached file testcase
Keywords: testcase
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.
Keywords: polish, pp
Whiteboard: [parity-IE] [parity-Chrome] [parity-Safari] → [parity-IE][parity-Chrome][parity-Safari][polish-interactive]
Whiteboard: [parity-IE][parity-Chrome][parity-Safari][polish-interactive] → [parity-IE][parity-Chrome][parity-Safari][polish-interactive][polish-p2]
Attached file :hover / :active testcase (obsolete) —
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.
This is the testcase I wanted to upload.
Attachment #441766 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
This is a combined diff of the four parts on

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.
Assignee: nobody → mstange
Attachment #441770 - Flags: review?(Olli.Pettay)
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.
Attached patch v2 (obsolete) — Splinter Review

(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.
Attachment #441770 - Attachment is obsolete: true
Attachment #441847 - Flags: review?(Olli.Pettay)
Attachment #441770 - Flags: review?(Olli.Pettay)
Attachment #441847 - Flags: review?(dbaron)
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?
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 on attachment 441847 [details] [diff] [review]

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


>+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

r=dbaron on that patch

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

>+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.
Attachment #441847 - Flags: review?(dbaron) → review-
(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.
Attached patch part 1, v3Splinter Review
Attachment #441847 - Attachment is obsolete: true
Attachment #441847 - Flags: review?(Olli.Pettay)
Attached patch part 2, v3Splinter Review
Attached patch part 3, v3 (obsolete) — Splinter Review
Attachment #442660 - Flags: review?(Olli.Pettay)
Attached patch part 4, v3 (obsolete) — Splinter Review
Attachment #442661 - Flags: review?(Olli.Pettay)
Attached patch part 5, v3Splinter Review
Attachment #442662 - Flags: review?(Olli.Pettay)
Comment on attachment 442660 [details] [diff] [review]
part 3, v3

I think this will become obsolete with bug 562932.
Attachment #442660 - Attachment is obsolete: true
Attachment #442660 - Flags: review?(Olli.Pettay)
Comment on attachment 442661 [details] [diff] [review]
part 4, v3

I'll have to update this on top of bug 562932, which adds nsIDOMNSLabelElement.
Attachment #442661 - Flags: review?(Olli.Pettay)
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).
Depends on: 562932
I guess this should be review after bug 562932.
Attachment #442662 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [parity-IE][parity-Chrome][parity-Safari][polish-interactive][polish-p2] → [parity-IE][parity-webkit][polish-interactive][polish-p2]
Attached patch part 4, v4Splinter Review
on top of bug 562932
Attachment #446016 - Flags: review?(Olli.Pettay)
Attachment #442661 - Attachment is obsolete: true
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
Attachment #446016 - Flags: review?(Olli.Pettay) → review+
Test fixup:
Flags: in-testsuite+
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a5
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?
(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.
Depends on: 626168
Blocks: 656379

I removed some outlines in gtk which causes some repaints not to show
up. However I think this test should be simplified a bit instead.

When this test landed in bug 426082, snapshots and such were the only
way to test this because it was a widget hack, effectively. Nowadays,
that "forward hover and active state from label to labeled element"
happens at the event state manager level, and thus we can test it much
more easily using simple selector-matching.

Comment on attachment 9151266 [details]
Simplify Bug 426082.html to not depend on paint events. r=mstange

Revision D76602 was moved to bug 1639057. Setting attachment 9151266 [details] to obsolete.

Attachment #9151266 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.