Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla1.9.3a5

Status

()

Core
Layout: Form Controls
--
enhancement
VERIFIED FIXED
9 years ago
3 months ago

People

(Reporter: Emil Ivanov, Assigned: mstange)

Tracking

({polish, pp, testcase})

Trunk
mozilla1.9.3a5
polish, pp, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-IE][parity-webkit][polish-interactive][polish-p2])

Attachments

(7 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
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.
(Reporter)

Comment 2

9 years ago
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

9 years ago
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...
(Reporter)

Comment 4

9 years ago
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

9 years ago
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?
(Reporter)

Comment 6

9 years ago
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

9 years ago
So pointing at the checkbox does NOT make it change state?
(Reporter)

Comment 8

9 years ago
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

9 years ago
OK.  Confirming, but someone on Windows will need to pick this up, I guess...
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

9 years ago
Version: unspecified → Trunk
(Reporter)

Updated

9 years ago
Blocks: 164421
(Reporter)

Updated

9 years ago
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.

Comment 11

9 years ago
Feature freeze for Firefox 3.1 (and this is definitely a feature) was months ago.
(Reporter)

Updated

9 years ago
Whiteboard: [parity-IE] [parity-chrome]

Updated

9 years ago
Flags: blocking1.9.2?
(Reporter)

Updated

9 years ago
Whiteboard: [parity-IE] [parity-chrome] → [parity-IE] [parity-Chrome] [parity-Safari]
(Reporter)

Comment 12

9 years ago
Safari 4 Beta go this too.

Comment 13

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

Comment 15

9 years ago
(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.
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-

Comment 16

8 years ago
Can this be blocking  Bug 251198 -  Bright, clear focus appearance for HTML content on Win/GTK platforms?
(Reporter)

Comment 17

8 years ago
Created attachment 431503 [details]
testcase
(Reporter)

Updated

8 years ago
Keywords: testcase

Comment 18

8 years ago
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.
(Reporter)

Updated

8 years ago
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]
(Assignee)

Comment 19

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

Comment 20

7 years ago
Created attachment 441768 [details]
:hover / :active testcase

This is the testcase I wanted to upload.
Attachment #441766 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 23

7 years ago
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.
Attachment #441770 - Attachment is obsolete: true
Attachment #441847 - Flags: review?(Olli.Pettay)
Attachment #441770 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

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

Comment 25

7 years ago
They're described in the commit messages on http://hg.mozilla.org/users/mstange_themasta.com/public-patchsets/pushloghtml?changeset=e7fdbbd9b2c8
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]
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.
Attachment #441847 - Flags: review?(dbaron) → review-
(Assignee)

Comment 28

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

Comment 29

7 years ago
Created attachment 442658 [details] [diff] [review]
part 1, v3
Attachment #441847 - Attachment is obsolete: true
Attachment #441847 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 30

7 years ago
Created attachment 442659 [details] [diff] [review]
part 2, v3
(Assignee)

Comment 31

7 years ago
Created attachment 442660 [details] [diff] [review]
part 3, v3
Attachment #442660 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 32

7 years ago
Created attachment 442661 [details] [diff] [review]
part 4, v3
Attachment #442661 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 33

7 years ago
Created attachment 442662 [details] [diff] [review]
part 5, v3
Attachment #442662 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 34

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

Comment 35

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

Updated

7 years ago
Attachment #442662 - Flags: review?(Olli.Pettay) → review+

Updated

7 years ago
Whiteboard: [parity-IE][parity-Chrome][parity-Safari][polish-interactive][polish-p2] → [parity-IE][parity-webkit][polish-interactive][polish-p2]
(Assignee)

Comment 38

7 years ago
Created attachment 446016 [details] [diff] [review]
part 4, v4

on top of bug 562932
Attachment #446016 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

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

Comment 40

7 years ago
http://hg.mozilla.org/mozilla-central/rev/0e98d12041d8
http://hg.mozilla.org/mozilla-central/rev/9083e999f3ec
http://hg.mozilla.org/mozilla-central/rev/fa721504fd11
http://hg.mozilla.org/mozilla-central/rev/171c0219adb3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 41

7 years ago
Test fixup:
http://hg.mozilla.org/mozilla-central/rev/3ba26104a2f4
Flags: in-testsuite+
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a5
(Reporter)

Comment 42

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

Comment 43

7 years ago
(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.
Status: RESOLVED → VERIFIED
Depends on: 626168

Updated

6 years ago
Blocks: 656379
You need to log in before you can comment on or make changes to this bug.