Closed Bug 943759 Opened 11 years ago Closed 11 years ago

UITour: Pointer-events should go through highlights

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox29 + fixed

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.
Not working on Windows either.
OS: Mac OS X → All
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)
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.
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)
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Priority: P4 → P3
Neil, if you have time to take this bug, feel free to steal it.
Assignee: MattN+bmo → enndeakin
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)
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.
I don't see any issue here. Whenever I hover over the blue highlight, the Customize button highlights in dark grey.
(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.
I just wanted to make sure the implementation matched what was expected.
Attachment #8365157 - Flags: feedback?(MattN+bmo) → feedback+
Attachment #8365157 - Flags: review?(neil)
Comment on attachment 8365157 [details] [diff] [review] Implement mouse transparency on all platforms windows/nsWindow.cpp/h changes
Attachment #8365157 - Flags: review?(jmathies)
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 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 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+
> 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.
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)
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.
Attached patch popup-pagehide.xul (deleted) — Splinter Review
The following xul testcase demonstrates how this works.
Flags: needinfo?(MattN+bmo)
(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.
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.
(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.
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.
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 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?
Attachment #8365157 - Flags: review?(karlt) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 969173
Blocks: 712184
Depends on: 993366
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: