Closed
Bug 1042163
Opened 10 years ago
Closed 10 years ago
Visual issues with the global indicator for screen/device sharing
Categories
(Firefox :: General, defect)
Tracking
()
People
(Reporter: phlsa, Assigned: florian)
References
Details
Attachments
(2 files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gijs
:
review+
davidb
:
a11y-review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
There are a few minor issues with the global sharing indicator implemented in bug 1037408. 1) I don't think the indicator is keyboard accessible, so we should remove the focus rings on the buttons 2) Could you move the entire indicator up by 1px so that the upper border is hidden? 3) There is a tiny square showing up below the indicator when you click one of the buttons.
Flags: firefox-backlog+
Reporter | ||
Comment 1•10 years ago
|
||
Screenshot of the small square I talked about in 3)
Comment 2•10 years ago
|
||
(In reply to Philipp Sackl [:phlsa] from comment #1) > Created attachment 8460349 [details] > Square below indicator > > Screenshot of the small square I talked about in 3) That looks like an empty menu. What are the STR to see this? Are there errors in the browser console? Is this self-built or regular nightly?
Flags: needinfo?(philipp)
Reporter | ||
Comment 3•10 years ago
|
||
STR on Windows 8 with latest Nightly: - Initiate any kind of sharing from http://queze.net/goinfre/ff_gum_test.html - Click the microphone/camera/screen button in the global indicator - Emtpy menu appears I also tested it with an empty profile. The browser console doesn't show any messages.
Flags: needinfo?(philipp)
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #2) > (In reply to Philipp Sackl [:phlsa] from comment #1) > > Created attachment 8460349 [details] > > Square below indicator > > > > Screenshot of the small square I talked about in 3) > > That looks like an empty menu. Yes, there's an empty menu, and we listen for the popupshowing event. When there are several tabs using the device we fill the menu; if there is only one, we return false (shouldn't that prevent the menu from being shown?) and show the doorhanger directly.
Comment 5•10 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #4) > (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #2) > > (In reply to Philipp Sackl [:phlsa] from comment #1) > > > Created attachment 8460349 [details] > > > Square below indicator > > > > > > Screenshot of the small square I talked about in 3) > > > > That looks like an empty menu. > > Yes, there's an empty menu, and we listen for the popupshowing event. When > there are several tabs using the device we fill the menu; if there is only > one, we return false (shouldn't that prevent the menu from being shown?) and > show the doorhanger directly. I'm sure that preventDefault() is meant to work, and I thought that returning false would always do the same as preventDefault - but http://stackoverflow.com/questions/1357118/event-preventdefault-vs-return-false and in particular http://stackoverflow.com/questions/1357118/event-preventdefault-vs-return-false#comment10263970_1357151 (whose link is sadly dead) seem to imply otherwise... in any case, it's obviously not working in practice, so we need to figure out why...
Comment 6•10 years ago
|
||
(In reply to Philipp Sackl [:phlsa] from comment #0) > There are a few minor issues with the global sharing indicator implemented > in bug 1037408. > > 1) I don't think the indicator is keyboard accessible, so we should remove > the focus rings on the buttons Or we should make it keyboard accessible... in fact, if you focus it, I expect tab etc. "just work" already...
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #5) > (In reply to Florian Quèze [:florian] [:flo] from comment #4) > > Yes, there's an empty menu, and we listen for the popupshowing event. When > > there are several tabs using the device we fill the menu; if there is only > > one, we return false (shouldn't that prevent the menu from being shown?) and > > show the doorhanger directly. > > I'm sure that preventDefault() is meant to work Right, if I had a Windows machine with me right now, I would just try to add a preventDefault call and see if it works before thinking more about this issue.
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #5) > I'm sure that preventDefault() is meant to work, and I thought that > returning false would always do the same as preventDefault [...] > http://stackoverflow.com/questions/1357118/event-preventdefault-vs-return- > false#comment10263970_1357151 (whose link is sadly dead) seem to imply > otherwise... Ah, the explanation there (that onpopupshowing="return false;" works but addEventListener("popupshowing", function() {return false;}) does nothing) seems reasonable.
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #6) > (In reply to Philipp Sackl [:phlsa] from comment #0) > > There are a few minor issues with the global sharing indicator implemented > > in bug 1037408. > > > > 1) I don't think the indicator is keyboard accessible, so we should remove > > the focus rings on the buttons > > Or we should make it keyboard accessible... in fact, if you focus it, I > expect tab etc. "just work" already... Hm, can you »tab« into the indicator? If you have to click first (which will then focus the tab) making it accessible doesn't seem necessary.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Philipp Sackl [:phlsa] from comment #9) > Hm, can you »tab« into the indicator? If you have to click first (which will > then focus the tab) making it accessible doesn't seem necessary. I can't, but I think screenreaders have keyboard shortcuts to go through the windows.
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #10) > (In reply to Philipp Sackl [:phlsa] from comment #9) > > > Hm, can you »tab« into the indicator? If you have to click first (which will > > then focus the tab) making it accessible doesn't seem necessary. > > I can't, but I think screenreaders have keyboard shortcuts to go through the > windows. OK, but screen readers don't need the (visual) focus ring, right?
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Philipp Sackl [:phlsa] from comment #0) > 1) I don't think the indicator is keyboard accessible, so we should remove > the focus rings on the buttons This patch removes the focus ring. I think it appeared only on Windows. > 2) Could you move the entire indicator up by 1px so that the upper border is > hidden? I think moving the entire indicator up by 1px would cause a 1px line to be displayed on an external screen if you have one placed above the primary screen. I tried removing the top border instead (window { border-top: none; }). Unfortunately that looked uglier than the current appearance because the window still have rounded corners at the top. I haven't found a way to have sqare corners at the top of the window. > 3) There is a tiny square showing up below the indicator when you click one > of the buttons. Fixed by the patch.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8461435 -
Flags: review?(gijskruitbosch+bugs)
Comment 13•10 years ago
|
||
Comment on attachment 8461435 [details] [diff] [review] Patch Review of attachment 8461435 [details] [diff] [review]: ----------------------------------------------------------------- This WFM, but we should check with David / someone for the a11y side of this.
Attachment #8461435 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8461435 -
Flags: review+
Attachment #8461435 -
Flags: a11y-review?(dbolter)
Comment 14•10 years ago
|
||
There are sighted users who require keyboard-only access. How do they use this feature?
Comment 15•10 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #10) > (In reply to Philipp Sackl [:phlsa] from comment #9) > > > Hm, can you »tab« into the indicator? If you have to click first (which will > > then focus the tab) making it accessible doesn't seem necessary. > > I can't, but I think screenreaders have keyboard shortcuts to go through the > windows. (In reply to David Bolter [:davidb] from comment #14) > There are sighted users who require keyboard-only access. How do they use > this feature? I think right now there is no non-third-party way to access the global indicator using the keyboard. It's technically "alternative UI" in the sense that everything it does could be done from within the Firefox window as well (although you would have to search for the tab/window where you are sharing yourself - perhaps we should expose this in the menu somewhere?). I don't think there is currently a cross platform way for us to do OS-level keyhandling in order to provide a shortcut to focus the global indicator. Even if there was, I imagine that discoverability of such a shortcut would be problematic.
Comment 16•10 years ago
|
||
Comment on attachment 8461435 [details] [diff] [review] Patch Review of attachment 8461435 [details] [diff] [review]: ----------------------------------------------------------------- a11y+ given the constraints as per IRC chat (thanks Gijs). Please file that follow up for the global indicator list a11y improvements.
Attachment #8461435 -
Flags: a11y-review?(dbolter) → a11y-review+
Comment 17•10 years ago
|
||
Hi Florian, can you provide a point value and mark the bug as [qa+] or [qa-] for verification.
Iteration: --- → 34.1
QA Whiteboard: [qa?]
Flags: needinfo?(florian)
Assignee | ||
Updated•10 years ago
|
Points: --- → 1
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(florian)
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
QA Contact: drno
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/49e78d49d321
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/49e78d49d321
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8461435 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: bug 1037408 [User impact if declined]: visual glitches on the global webrtc sharing indicator. [Describe test coverage new/current, TBPL]: currently in m-c. [Risks and why]: Low, self contained change. [String/UUID change made/needed]: none.
Attachment #8461435 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Iteration: 34.1 → 34.2
Updated•10 years ago
|
status-firefox33:
--- → affected
status-firefox34:
--- → fixed
Updated•10 years ago
|
Attachment #8461435 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6a784dd3987
Comment 22•10 years ago
|
||
Florin in case you don't know yet, this page http://mozilla.github.io/webrtc-landing/gum_test.html allows you to test screen sharing now (at least the getUserMedia part of it).
QA Contact: drno → florin.mezei
Updated•10 years ago
|
QA Contact: florin.mezei → paul.silaghi
Comment 23•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #15) > I think right now there is no non-third-party way to access the global > indicator using the keyboard. It's technically "alternative UI" in the sense > that everything it does could be done from within the Firefox window as well > (although you would have to search for the tab/window where you are sharing > yourself - perhaps we should expose this in the menu somewhere?). Does this also address the case when pressing 'tab' after focusing the indicator with the mouse first?
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #23) > (In reply to :Gijs Kruitbosch from comment #15) > > I think right now there is no non-third-party way to access the global > > indicator using the keyboard. It's technically "alternative UI" in the sense > > that everything it does could be done from within the Firefox window as well > > (although you would have to search for the tab/window where you are sharing > > yourself - perhaps we should expose this in the menu somewhere?). > Does this also address the case when pressing 'tab' after focusing the > indicator with the mouse first? Depends what you mean by "address". I think the patch here is hiding the focus ring unconditionally.
Comment 25•10 years ago
|
||
I'm talking about the keyboard navigation
Comment 26•10 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #25) > I'm talking about the keyboard navigation in any other way possible
Assignee | ||
Comment 27•10 years ago
|
||
Figuring out a keyboard-accessible solution is bug 1043372.
Comment 28•10 years ago
|
||
Thanks. The focus ring and the small squares are gone now. Verified fixed 34.0a1 (2014-08-12), 33.0a2 (2014-08-13) Win 7, Ubuntu 13.04.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Comment 29•10 years ago
|
||
Couple of other issues, please tell if I should file any separately: 1. On Windows, when hovering over the first icon in the indicator (FF logo), the color is changing to a dark gray. On Linux, the color doesn't change. 2. On Windows, when hovering over the 2nd, 3rd icons, the color is changing to a dark orange. The color is changing back after the mouse cursor moves aside. On linux, the color remains stuck to dark orange after moving the cursor away.
Flags: needinfo?(florian)
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•