1042163 - Visual issues with the global indicator for screen/device sharing

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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.

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Square below indicator

Screenshot of the small square I talked about in 3)

Comment 2

[:Gijs (he/him)]

(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?

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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.

Comment 4

[Florian Quèze [:florian]]

(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

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

(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...

Comment 7

[Florian Quèze [:florian]]

(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.

Comment 8

[Florian Quèze [:florian]]

(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.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(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.

Comment 10

[Florian Quèze [:florian]]

(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.

Comment 11

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(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?

Comment 12

[Florian Quèze [:florian]]

Attachment: Patch

(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.

Comment 13

[:Gijs (he/him)]

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.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

There are sighted users who require keyboard-only access. How do they use this feature?

Comment 15

[:Gijs (he/him)]

(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

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 17

[Marco Mucci [:MarcoM]]

Hi Florian, can you provide a point value and mark the bug as [qa+] or [qa-] for verification.

Comment 18

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/49e78d49d321

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49e78d49d321

Comment 20

[Florian Quèze [:florian]]

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.

Comment 21

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6a784dd3987

Comment 22

[Nils Ohlmeier [:drno]]

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).

Comment 23

[Paul Silaghi, QA [:pauly]]

(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?

Comment 24

[Florian Quèze [:florian]]

(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

[Paul Silaghi, QA [:pauly]]

I'm talking about the keyboard navigation

Comment 26

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi, QA [:pauly] from comment #25)
> I'm talking about the keyboard navigation
in any other way possible

Comment 27

[Florian Quèze [:florian]]

Figuring out a keyboard-accessible solution is bug 1043372.

Comment 28

[Paul Silaghi, QA [:pauly]]

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.

Comment 29

[Paul Silaghi, QA [:pauly]]

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.

Comment 30

[Florian Quèze [:florian]]

This seems like a Linux bug, yes.