Closed
Bug 426082
Opened 16 years ago
Closed 14 years ago
Hovering on label should indicate for which checkbox/radiobutton belongs to
Categories
(Core :: Layout: Form Controls, enhancement)
Core
Layout: Form Controls
Tracking
()
VERIFIED
FIXED
mozilla1.9.3a5
People
(Reporter: stream, Assigned: mstange)
References
Details
(Keywords: platform-parity, polish, testcase, Whiteboard: [parity-IE][parity-webkit][polish-interactive][polish-p2])
Attachments
(7 files, 6 obsolete files)
15.72 KB,
image/png
|
Details | |
356 bytes,
text/html
|
Details | |
685 bytes,
text/html
|
Details | |
4.65 KB,
patch
|
Details | Diff | Splinter Review | |
9.15 KB,
patch
|
Details | Diff | Splinter Review | |
5.51 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
7.49 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•16 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•16 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•16 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•16 years ago
|
||
OK then could this belong for OS integration, since IE does that on pages and everywhere in windows is like that.
Comment 5•16 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•16 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•16 years ago
|
||
So pointing at the checkbox does NOT make it change state?
Reporter | ||
Comment 8•16 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•16 years ago
|
||
OK. Confirming, but someone on Windows will need to pick this up, I guess...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Updated•16 years ago
|
Version: unspecified → Trunk
Reporter | ||
Updated•16 years ago
|
Severity: major → enhancement
Comment 10•16 years ago
|
||
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•16 years ago
|
||
Feature freeze for Firefox 3.1 (and this is definitely a feature) was months ago.
Reporter | ||
Updated•16 years ago
|
Whiteboard: [parity-IE] [parity-chrome]
Updated•15 years ago
|
Flags: blocking1.9.2?
Reporter | ||
Updated•15 years ago
|
Whiteboard: [parity-IE] [parity-chrome] → [parity-IE] [parity-Chrome] [parity-Safari]
Reporter | ||
Comment 12•15 years ago
|
||
Safari 4 Beta go this too.
Comment 13•15 years ago
|
||
(In reply to comment #12) > Safari 4 Beta go this too. Appearently not on a Mac
Comment 14•15 years ago
|
||
(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•15 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•15 years ago
|
||
Can this be blocking Bug 251198 - Bright, clear focus appearance for HTML content on Win/GTK platforms?
Reporter | ||
Comment 17•14 years ago
|
||
Comment 18•14 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•14 years ago
|
Updated•14 years ago
|
Whiteboard: [parity-IE][parity-Chrome][parity-Safari][polish-interactive] → [parity-IE][parity-Chrome][parity-Safari][polish-interactive][polish-p2]
Assignee | ||
Comment 19•14 years ago
|
||
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•14 years ago
|
||
This is the testcase I wanted to upload.
Attachment #441766 -
Attachment is obsolete: true
Assignee | ||
Comment 21•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 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•14 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•14 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•14 years ago
|
||
Attachment #441847 -
Attachment is obsolete: true
Attachment #441847 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 30•14 years ago
|
||
Assignee | ||
Comment 31•14 years ago
|
||
Attachment #442660 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 32•14 years ago
|
||
Attachment #442661 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 33•14 years ago
|
||
Attachment #442662 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 34•14 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•14 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)
Comment 36•14 years ago
|
||
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
Comment 37•14 years ago
|
||
I guess this should be review after bug 562932.
Updated•14 years ago
|
Attachment #442662 -
Flags: review?(Olli.Pettay) → review+
Updated•14 years ago
|
Whiteboard: [parity-IE][parity-Chrome][parity-Safari][polish-interactive][polish-p2] → [parity-IE][parity-webkit][polish-interactive][polish-p2]
Assignee | ||
Comment 38•14 years ago
|
||
on top of bug 562932
Attachment #446016 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 years ago
|
Attachment #442661 -
Attachment is obsolete: true
Comment 39•14 years ago
|
||
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•14 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
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•14 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•14 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•14 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
Comment 44•4 years ago
|
||
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 45•4 years ago
|
||
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.
Description
•