Closed
Bug 943759
Opened 11 years ago
Closed 11 years ago
UITour: Pointer-events should go through highlights
Categories
(Firefox :: General, defect, P3)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 29
People
(Reporter: MattN, Assigned: enndeakin)
References
(Depends on 1 open bug, )
Details
Attachments
(3 files)
This isn't working properly on OS X. I'm not sure about other platforms. This is preferred for the proposed Australis update flow as the user can start by opening the menu panel while the menu button is highlighted. Worst case, the user can click the "Show me more!" button.
Reporter | ||
Comment 2•11 years ago
|
||
Enn, we have a <panel>[1] that we want to pointer-events to go through (e.g. pointer-events: none). Do you know of a way to do this or how hard it would be to make it work?
[1] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul?rev=0d1e734bbd0c#208
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 3•11 years ago
|
||
The workaround we can fallback to is re-dispatching events from the panel to the panel's anchor since the panel normally doesn't overlap other elements much.
Assignee | ||
Comment 4•11 years ago
|
||
There's support for having mouse events pass through panels already on Windows and Mac but not on Linux, although it's only currently available for drag feedback popups, (the translucent images that appear when dragging).
http://mxr.mozilla.org/mozilla-central/ident?i=mIsDragPopup lists places which would need to be changed to be extended to other types of popups.
Flags: needinfo?(enndeakin)
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Priority: P4 → P3
Reporter | ||
Comment 5•11 years ago
|
||
Neil, if you have time to take this bug, feel free to steal it.
Reporter | ||
Updated•11 years ago
|
Assignee: MattN+bmo → enndeakin
Assignee | ||
Comment 6•11 years ago
|
||
In layout/xul, we allow <panel mousethrough="always"> to be used as a way to ignore mouse events on the window. We could extend it to support pointer-events but adding that doesn't really gain anything. nsXULPopupManager::GetVisiblePopups is changed so that this can be supported on all popups.
Thie patch includes an implementation of this using input shapes on gtk and fixes mouse events on Windows. Mac needs no extra fixes.
Attachment #8365157 -
Flags: feedback?(MattN+bmo)
Reporter | ||
Comment 7•11 years ago
|
||
I did a quick test on OS X and found an issue with hover styling when the customize mode icon is highlighted (step 2/4 in the tour) . When I hover over the highlight, the button doesn't get the hover state like it does if I move the cursor to the side of the highlight.
Assignee | ||
Comment 8•11 years ago
|
||
I don't see any issue here. Whenever I hover over the blue highlight, the Customize button highlights in dark grey.
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to Neil Deakin from comment #8)
> I don't see any issue here. Whenever I hover over the blue highlight, the
> Customize button highlights in dark grey.
OK, I'm on Mavericks (10.9) when I'm seeing this. I guess we can go ahead with this patch. Do you want me to test on the other platforms or what kind of feedback do you want? @mousethrough="always" is fine instead of pointer-events: none.
Assignee | ||
Comment 10•11 years ago
|
||
I just wanted to make sure the implementation matched what was expected.
Reporter | ||
Updated•11 years ago
|
Attachment #8365157 -
Flags: feedback?(MattN+bmo) → feedback+
Assignee | ||
Updated•11 years ago
|
Attachment #8365157 -
Flags: review?(neil)
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8365157 [details] [diff] [review]
Implement mouse transparency on all platforms
windows/nsWindow.cpp/h changes
Attachment #8365157 -
Flags: review?(jmathies)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8365157 [details] [diff] [review]
Implement mouse transparency on all platforms
GTK changes. This works but not sure if there's a better way to accomplish this. I realize I should be passing rect to cairo_region_create_rectangle so I'll fix that in the next iteration.
Attachment #8365157 -
Flags: review?(karlt)
Comment 13•11 years ago
|
||
Comment on attachment 8365157 [details] [diff] [review]
Implement mouse transparency on all platforms
>+ // A drag popup may be used for non-static translucent drag feedback
>+ bool isDragPopup = false;
>+ if (mPopupType == ePopupTypePanel &&
>+ mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
>+ nsGkAtoms::drag, eIgnoreCase)) {
>+ widgetData.mIsDragPopup = true;
Where does this get used? Also while I'm here, where do drag popups get created? (I couldn't find where the type="drag" attribute gets set.)
>+ // Don't pass events to popups that ignore mouse events.
>+ if (aBuilder->IsForEventDelivery() && mMouseTransparent) {
Won't the builder handle the mouse transparency case already?
Comment 14•11 years ago
|
||
Comment on attachment 8365157 [details] [diff] [review]
Implement mouse transparency on all platforms
Review of attachment 8365157 [details] [diff] [review]:
-----------------------------------------------------------------
::: widget/windows/nsWindow.cpp
@@ +362,5 @@
> mForeground = ::GetSysColor(COLOR_WINDOWTEXT);
>
> mTaskbarPreview = nullptr;
> mHasTaskbarIconBeenCreated = false;
> + mMouseTransparent = false;
nit - could you move these two bools up to where we init the result of the boolean member variables.
Attachment #8365157 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 15•11 years ago
|
||
> Where does this get used? Also while I'm here, where do drag popups get
> created? (I couldn't find where the type="drag" attribute gets set.)
I don't think it ended up being used in Firefox currently, although it was going to be used for tab dragging.
> Won't the builder handle the mouse transparency case already?
Well, it won't work for drag popups. But maybe they should just explicitly set mousethrough then, so I expect I can now just remove the BuildDisplayList override there.
Comment 16•11 years ago
|
||
Can you tell me how to run the UITour, please?
Do I need to find login details for https://docs.google.com/a/mozilla.com/file/d/0B60Ayj_R5h_jb0gzSWtpSkpSeXM/edit ?
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 17•11 years ago
|
||
1. Set the browser.uitour.whitelist.add.testing preference to www-demo3.allizom.org
2. Load https://www-demo3.allizom.org/b/en-US/firefox/australis/update/
I needed to create a new profile to get it to work.
Assignee | ||
Comment 18•11 years ago
|
||
The following xul testcase demonstrates how this works.
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(MattN+bmo)
Comment 19•11 years ago
|
||
(In reply to Neil Deakin from comment #15)
> > Where does this get used? Also while I'm here, where do drag popups get
> > created? (I couldn't find where the type="drag" attribute gets set.)
>
> I don't think it ended up being used in Firefox currently, although it was
> going to be used for tab dragging.
How would tab dragging need the widget-level flag?
> > Won't the builder handle the mouse transparency case already?
>
> Well, it won't work for drag popups. But maybe they should just explicitly
> set mousethrough then, so I expect I can now just remove the
> BuildDisplayList override there.
Comment 20•11 years ago
|
||
Accidental submit, sorry.
(In reply to Neil Deakin from comment #15)
> > Where does this get used? Also while I'm here, where do drag popups get
> > created? (I couldn't find where the type="drag" attribute gets set.)
>
> I don't think it ended up being used in Firefox currently, although it was
> going to be used for tab dragging.
How would tab dragging need the widget-level flag, which gets thrown away almost immediately?
> > Won't the builder handle the mouse transparency case already?
>
> Well, it won't work for drag popups. But maybe they should just explicitly
> set mousethrough then, so I expect I can now just remove the
> BuildDisplayList override there.
Yes, I would appreciate it if you did that.
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #20)
> Accidental submit, sorry.
>
> (In reply to Neil Deakin from comment #15)
> > > Where does this get used? Also while I'm here, where do drag popups get
> > > created? (I couldn't find where the type="drag" attribute gets set.)
> >
> > I don't think it ended up being used in Firefox currently, although it was
> > going to be used for tab dragging.
>
> How would tab dragging need the widget-level flag, which gets thrown away
> almost immediately?
>
Tab dragging mostly needed the mousethrough part. With the patch, type="drag" only serves to hint to gtk that the popup is a drag popup. See http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsWindow.cpp#3527 so it now doesn't do anything specific on other platforms.
Reporter | ||
Comment 22•11 years ago
|
||
FYI, we would really like this for the initial Aurora 29 release as the UITour will be used at that point (bug 966014) and this is a usability issue hindering that. Let me know if that isn't do-able.
tracking-firefox29:
--- → ?
Updated•11 years ago
|
Comment 23•11 years ago
|
||
Comment on attachment 8365157 [details] [diff] [review]
Implement mouse transparency on all platforms
Ah, I hadn't realised that there was still one consumer left. In that case, r=me if you add explicit mousethough to drag popups and remove the BuildDisplayList override.
Attachment #8365157 -
Flags: review?(neil) → review+
Comment 24•11 years ago
|
||
Comment on attachment 8365157 [details] [diff] [review]
Implement mouse transparency on all platforms
>+ // true if the window should be transparent to mouse events
>+ bool mMouseTransparent;
Can you document that this is only effective for eWindowType_popup, please?
Updated•11 years ago
|
Attachment #8365157 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 25•11 years ago
|
||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/af70f4108299
https://hg.mozilla.org/mozilla-central/rev/ed29e233ee6b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox29:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in
before you can comment on or make changes to this bug.
Description
•