Closed Bug 1042163 Opened 6 years ago Closed 6 years ago

Visual issues with the global indicator for screen/device sharing

Categories

(Firefox :: General, defect)

33 Branch
x86
All
defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.2
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: phlsa, Assigned: florian)

References

Details

Attachments

(2 files)

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+
Attached image Square below indicator
Screenshot of the small square I talked about in 3)
(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)
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)
(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.
Blocks: 1040061
(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...
(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...
(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.
(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.
(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.
(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 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?
Attached patch PatchSplinter Review
(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 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)
There are sighted users who require keyboard-only access. How do they use this feature?

(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 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+
Depends on: 1043372
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)
Points: --- → 1
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(florian)
Whiteboard: [sceensharing-uplift]
https://hg.mozilla.org/mozilla-central/rev/49e78d49d321
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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?
Iteration: 34.1 → 34.2
Attachment #8461435 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
QA Contact: florin.mezei → paul.silaghi
(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?
(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.
I'm talking about the keyboard navigation
(In reply to Paul Silaghi, QA [:pauly] from comment #25)
> I'm talking about the keyboard navigation
in any other way possible
Figuring out a keyboard-accessible solution is bug 1043372.
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!]
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)
This seems like a Linux bug, yes.
Flags: needinfo?(florian)
Whiteboard: [sceensharing-uplift]
You need to log in before you can comment on or make changes to this bug.