1132678 - Make #page-report-button into a button (instead of an image)

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[:Margaret Leibovic]

This is inspired by bug 1131458. The solution for this bug will probably look similar.

Gijs, I'm making you a mentor, I hope that's okay :)

Comment 1

[Manraj Singh [:manrajsingh]]

Hi

I would like to take this up as my first bug.Please someone assign me the same and also how to reproduce this bug.

Comment 2

[:Margaret Leibovic]

Hi Manraj. First of all, do you have a build set up? If not, you should follow the instructions here:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

After that, you should look at the patch in bug 1131458 to see how we converted #reader-mode-button into a <toolbarbutton>, and do the same for the #page-report-button.

Gijs is more qualified to give advice on the specifics of desktop Firefox, since I'm more familiar with Firefox for Android.

Comment 3

[Manraj Singh [:manrajsingh]]

Hi Ma'am

Yes I have the build set up ready and have followed the tutorials given in https://codefirefox.com . Okay I'll have a look at the patch. Please assign me this bug. Thank you.

Comment 4

[:Gijs (he/him)]

Hi Manraj,

Basically, we have the icon here:

http://hg.mozilla.org/mozilla-central/annotate/5de3af90c494/browser/base/content/browser.xul#l825

Interestingly, it might already get the right accessible role because it's a XUL image with an onclick handler... try adding a "tabbable" class to it and see if you can tab to it and activate it ("click" it, but without using the mouse) with the keyboard? If that doesn't work, we probably have to change it from an image into a toolbarbutton.

Let me know if you have any questions!

Comment 5

[Manraj Singh [:manrajsingh]]

Hi Sir

Sir I tried adding "tabbable" class here:

http://hg.mozilla.org/mozilla-central/annotate/5de3af90c494/browser/base/content/browser.xul#l826

making change from class="urlbar-icon" to class="tabbable urlbar-icon" but I still could not reach the blue popup using just keyboard.
http://puu.sh/g6OhX/e5d2baae7e.png

I could not verify if the class actually changed of the image since I found DOM Inspector is not working in Nightly on mach run.Any work around for it? Also if it did apply, I feel it should be converted to a button.

Comment 6

[Manraj Singh [:manrajsingh]]

Hi Sir

I got the DOM inspector working and can confirm adding "tabbable" class didn't fix the problem.

Comment 7

[:Gijs (he/him)]

Hey Manraj,

Thanks for trying this option! I am traveling today, I will look at this again first thing tomorrow morning. Sorry!

In the meantime, if you have time to try the route of turning it into a toolbarbutton, feel free to give that a go and let me know if it works and/or where you get stuck... :-)

Comment 8

[:Gijs (he/him)]

Right, so then let's convert this to a proper toolbarbutton.

You should basically change the node from "image" to "toolbarbutton". You probably also will need to add an "onkeypress" event handler to handle 'space' and 'enter' keypresses on the button to do the same thing as clicking it. It probably makes sense to rename the "onReportButtonClick" method here:

http://hg.mozilla.org/mozilla-central/annotate/056c556b41b2/browser/base/content/browser.js#l459

to "onReportButtonEvent" and to make it deal with both clicks and keypresses. You can look at the code here to see how to do so:

http://hg.mozilla.org/mozilla-central/rev/0b63903a6f41#l4.39

Keep in mind you will also need to keep the check for the target element that is already in the function.

You will still need to add the "tabbable" class on the toolbarbutton for this approach to work.

Feel free to ping me here or on bugzilla if you have more questions!

Comment 9

[Manraj Singh [:manrajsingh]]

Attachment: rev1 - Made #page-report-button tabbable and accessible via mouse as well as keyboard(space and enter).

Comment 10

[:Gijs (he/him)]

Comment on attachment 8568145 [details] [diff] [review]
rev1 - Made #page-report-button tabbable and accessible via mouse as well as keyboard(space and enter).

Review of attachment 8568145 [details] [diff] [review]:
-----------------------------------------------------------------

There are a bunch of nits here, so I'd like to see this again with those fixed. Also, have you tested the styling of the button? Does it look OK just like this, when focused and active and so on? On which platforms did you check?

::: browser/base/content/browser.js
@@ +460,5 @@
>    {
> +    if ((aEvent.type == "click" && aEvent.button != 0) || 
> +		(aEvent.target != this._reportButton) || 
> +		(aEvent.type == "keypress" && aEvent.charCode != Ci.nsIDOMKeyEvent.DOM_VK_SPACE &&
> +		aEvent.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN)){

Nits:
- please use spaces for indenting, no tabs
- please make sure the indenting of the 2nd-4th line of the if condition matches the position of the ( in the first line of the if condition
- there is trailing whitespace at the end of the first two lines of this if condition
- please leave a space after the final closing ) of the if condition, before the opening { of the block.

@@ +461,5 @@
> +    if ((aEvent.type == "click" && aEvent.button != 0) || 
> +		(aEvent.target != this._reportButton) || 
> +		(aEvent.type == "keypress" && aEvent.charCode != Ci.nsIDOMKeyEvent.DOM_VK_SPACE &&
> +		aEvent.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN)){
> +      return; //Left,Space or Enter only

Please fix this comment to say something like "We're only interested in left click and space and enter keypresses

@@ +462,5 @@
> +		(aEvent.target != this._reportButton) || 
> +		(aEvent.type == "keypress" && aEvent.charCode != Ci.nsIDOMKeyEvent.DOM_VK_SPACE &&
> +		aEvent.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN)){
> +      return; //Left,Space or Enter only
> +	}

Nit: align the brace correctly, please. (and again, spaces, no tabs)

::: browser/base/content/browser.xul
@@ +827,4 @@
>                         hidden="true"
>                         tooltiptext="&pageReportIcon.tooltip;"
> +                       onclick="gPopupBlockerObserver.onReportButtonEvent(event);"
> +					   onkeypress="gPopupBlockerObserver.onReportButtonEvent(event);"/>

Nit: please fix the indentation here.

Comment 11

[Manraj Singh [:manrajsingh]]

Attachment: rev2 - Made #page-report-button tabbable and accessible via mouse as well as keyboard(space and enter). Fixed whitespaces and indentations.

Hi Sir

I have fixed all the white spaces and indentations. Sorry for using tab spaces. Will not repeat the same mistake again. If there are any other left please let me know.

Regarding the style of button, no sir its not the same. Due to node change from image to toolbarbutton, it now has a blue background and border onhover and active.(Checked on Windows platform)

https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#29
https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#22

Also sir, one more thing I noticed is that when the popup opens, clicking button again reopens the popup instead of closing it. Need to find a workaround.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8568762 [details] [diff] [review]
rev2 - Made #page-report-button tabbable and accessible via mouse as well as keyboard(space and enter). Fixed whitespaces and indentations.

Review of attachment 8568762 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely getting closer!

So we should make sure that the styling doesn't change (besides showing focus, of course). On Windows, I think you basically want to do the same as this:

https://hg.mozilla.org/integration/fx-team/rev/0b63903a6f41#l5.11

in particular, you want to add -moz-appearance: none; and padding: 0, and also border: 0 none;

I don't think the two lines you linked to should be applying to this button - it doesn't have the .toolbarbutton-1 class, right? See if the above CSS helps fix this, and also if you need to add styling to show the focus outline like https://hg.mozilla.org/integration/fx-team/rev/0b63903a6f41#l7.22 does.

::: browser/base/content/browser.js
@@ +460,2 @@
>    {
> +    if ((aEvent.type == "click" && aEvent.button != 0) || 

Nit: still a trailing space here.

@@ +463,5 @@
> +        (aEvent.type == "keypress" && aEvent.charCode != Ci.nsIDOMKeyEvent.DOM_VK_SPACE &&
> +         aEvent.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN)) {
> +        return; // We're only interested in left click and space and enter keypresses
> +    }
> +    

Nit: and some trailing whitespace here.

@@ +468,2 @@
>      document.getElementById("blockedPopupOptions")
>              .openPopup(this._reportButton, "after_end", 0, 2, false, false, aEvent);

Well spotted that this doesn't work right. So this should check whether the popup is open. The easiest way to do this is to replace these two lines with:

let popup = document.getElementById("blockedPopupOptions");
if (popup.state == "closed") {
  popup.openPopup(this._reportButton, "after_end", 0, 2, false, false, aEvent);
} else {
  popup.hidePopup();
}

That should work, I think.

::: browser/base/content/browser.xul
@@ +822,5 @@
>                  <label class="urlbar-display urlbar-display-switchtab" value="&urlbar.switchToTab.label;"/>
>                </box>
>                <hbox id="urlbar-icons">
> +                <toolbarbutton id="page-report-button"
> +                       class="tabbable urlbar-icon"

Great, no more tabs!

Can you also align the attribute names with the first attribute (id, in the previous line) like the reader-mode-button has done?

Comment 13

[Manraj Singh [:manrajsingh]]

Attachment: Tried fixing CSS and button bug. Need help.

Hi Sir,

I couldn't locate the white space from line 463 to 467 mentioned. If it was the line left blank, I have removed it.

Regarding CSS, I added the two properties mentioned and it fixed the background but showed dotted border on :hover and :active, so added border and outline properties to fix. 

One bug that persist is slight shift of button to right on :hover and :active state. I tried adding property padding: 0; but didn't fix it.

I tried the solution for fixing re-click to close but it didn't fix it. On console logging, I noticed the popup state always remained in "closed" which means on re-clicking it always opened a new one.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8569384 [details] [diff] [review]
Tried fixing CSS and button bug. Need help.

Review of attachment 8569384 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +461,5 @@
> +    if ((aEvent.type == "click" && aEvent.button != 0) ||
> +        (aEvent.target != this._reportButton) ||
> +        (aEvent.type == "keypress" && aEvent.charCode != Ci.nsIDOMKeyEvent.DOM_VK_SPACE &&
> +         aEvent.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN)) {
> +        return; // We're only interested in left click and space and enter keypresses

Nit-picking: Sorry, I should have noticed this earlier: 2 spaces of indentation rather than 4, please... :-)

@@ +466,5 @@
> +    }
> +    let popup = document.getElementById("blockedPopupOptions");
> +    if (popup.state == "closed") {
> +        popup.openPopup(this._reportButton, "after_end", 0, 2, false, false, aEvent);
> +    } else {

You're absolutely right that this doesn't work. That surprises me a bit; sorry I didn't see it coming. It seems like the native popup handling already takes care of closing the popup before we get here. That's unfortunate. There isn't really an easy way to fix this... but it's also not a change compared to how the button used to work (so your changes didn't break anything).

Because of this, I think for now we should just remove the if...else again and keep only:

popup.openPopup(this._reportButton, "after_end", 0, 2, false, false, aEvent);

and we can file a new, seaprate bug to fix this behaviour.

::: browser/themes/windows/browser.css
@@ +1563,5 @@
>  /* popup blocker button */
>  
>  #page-report-button {
> +  -moz-appearance: none;
> +  padding: 0;

This should also get border-width: 0;

@@ +1571,5 @@
>  
>  #page-report-button:hover {
> +  outline: 0;
> +  border: 0;
> +  padding: 0;

And then you shouldn't need these

@@ +1579,5 @@
>  #page-report-button:hover:active,
>  #page-report-button[open="true"] {
> +  outline: 0;
> +  border: 0;
> +  padding: 0;

... or these.

Comment 15

[Manraj Singh [:manrajsingh]]

Attachment: rev3 - CSS fixed. Whitespaces removed. Tested on Windows. Result: Ok

Hi Sir :)

Sir I have removed/reduced the whitespaces mentioned.

Also I have added the border-width property and it fixed the problem. I haven't removed border and outline properties in :hover and :active because on removing selection a 1px dotted border appeared around the button.

I have extended this CSS for Linux and OSX but not tested. Need to check this.

Lastly, the re-click bug persist even after trying some other methods as well. Need to file another bug for it till a solution is found. :)

Comment 16

[:Gijs (he/him)]

(In reply to Manraj Singh [:manrajsingh] from comment #15)
> Created attachment 8569989 [details] [diff] [review]
> rev3 - CSS fixed. Whitespaces removed. Tested on Windows. Result: Ok
> 
> Hi Sir :)
> 
> Sir I have removed/reduced the whitespaces mentioned.
> 
> Also I have added the border-width property and it fixed the problem. I
> haven't removed border and outline properties in :hover and :active because
> on removing selection a 1px dotted border appeared around the button.

This surprises me, unless you mean the focus rectangle, which is expected (and should be there, because otherwise we don't know what the focused element is). I would like to look at this, but unfortunately I won't be back near my Windows machine until Monday. I'll ask Jared if he has time to take a look.

> I have extended this CSS for Linux and OSX but not tested. Need to check
> this.

OK. The CSS on OS X needs a bit of tweaking, I'll check Linux as well and upload a modified patch for you.

> Lastly, the re-click bug persist even after trying some other methods as
> well. Need to file another bug for it till a solution is found. :)

Yup.

Comment 17

[:Gijs (he/him)]

Attachment: Patch v4: update Linux & OS X styling

Jared, do you have time to look at the Windows side of this before Monday?

Comment 18

[Manraj Singh [:manrajsingh]]

Hi sir :)

Sir I am linking an image showing both on keeping and removing the outline and border properties. :)

http://puu.sh/geWrT/47f181b319.jpg

Also, should I file the bug on re-click to close? I tried fixing it but the state is changing to "closed" too quickly.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8570458 [details] [diff] [review]
Patch v4: update Linux & OS X styling

Review of attachment 8570458 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/browser.css
@@ +1569,5 @@
>    list-style-image: url("chrome://browser/skin/urlbar-popup-blocked.png");
>    -moz-image-region: rect(0, 16px, 16px, 0);
>  }
>  
>  #page-report-button:hover {

This should also have the #page-report-button:focus selector so that the icon changes when the keyboard is used to move focus.

We may also want to put the dotted outline for the focused button, but maybe just changing the icon to match the "hover" state will be sufficient.

Comment 20

[Manraj Singh [:manrajsingh]]

Attachment: Patch v5: Fixed Windows UI.

Hi Sir, :)

I have made the above changes. It now shows a 1px dotted lines around it.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8570898 [details] [diff] [review]
Patch v5: Fixed Windows UI.

Review of attachment 8570898 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. There is some bug where the :hover:active styling is not released when tabbing to another element after opening and closing the menu through the keyboard. The styling reverts after moving the mouse though.

Comment 22

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> Comment on attachment 8570898 [details] [diff] [review]
> Patch v5: Fixed Windows UI.
> 
> Review of attachment 8570898 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. There is some bug where the :hover:active styling is not
> released when tabbing to another element after opening and closing the menu
> through the keyboard. The styling reverts after moving the mouse though.

Yeah, this happens on the reading mode one as well, see bug 1133732.

Comment 23

[Manraj Singh [:manrajsingh]]

Hi sir

Sir patch works well, right? If yes, could you please check in. Thank you. :)

Comment 24

[:Gijs (he/him)]

Thanks! Landed with an updated commit message:

remote:   https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=2b22a22b1cc5

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b22a22b1cc5

Comment 26

[Dão Gottwald [:dao]]

Why does this patch set padding:0 on the button? There's supposed to be room between multiple urlbar icons, hence this code: http://hg.mozilla.org/mozilla-central/annotate/56492f7244a9/browser/themes/linux/browser.css#l913

Also, why do we need this button to be keyboard accessible? We show a full-fledged notification bar for blocked popups.

Comment 27

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #26)
> Why does this patch set padding:0 on the button? There's supposed to be room
> between multiple urlbar icons, hence this code:
> http://hg.mozilla.org/mozilla-central/annotate/56492f7244a9/browser/themes/
> linux/browser.css#l913

Please file a separate bug about this, because if there needs to be a change there, we'll need to do the same for the reader-mode button. I expect we can't use the default padding for a toolbar button here, though.

> Also, why do we need this button to be keyboard accessible? We show a
> full-fledged notification bar for blocked popups.

Parity with the other icons? Seems like a more appropriate question would be "why do we have a URL bar icon for blocked popups if we show a notification bar as well" (or why do we have a notification bar... etc.).

Comment 28

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #27)
> (In reply to Dão Gottwald [:dao] from comment #26)
> > Why does this patch set padding:0 on the button? There's supposed to be room
> > between multiple urlbar icons, hence this code:
> > http://hg.mozilla.org/mozilla-central/annotate/56492f7244a9/browser/themes/
> > linux/browser.css#l913
> 
> Please file a separate bug about this, because if there needs to be a change
> there, we'll need to do the same for the reader-mode button. I expect we
> can't use the default padding for a toolbar button here, though.

The reader mode button doesn't even use the urlbar-icon class. Margaret, can you take care of this?

> > Also, why do we need this button to be keyboard accessible? We show a
> > full-fledged notification bar for blocked popups.
> 
> Parity with the other icons?

This is misleading, since those icons have been images until recently. Most icons provided by add-ons are likely images still. So "the other icons" really only means the reader mode button.

The expectation so far has been that there's some other nicely accessible way provided, e.g. a menu item, a notification bar, etc.. It looks like the reader mode button breaks this (I would have expected a menu item under the View menu), but that doesn't seem like a good reason to go ahead with this bug.

I suggest that we back this out and wontfix the bug, since its gain is highly questionable, it adds code complexity and introduces the aforementioned styling issue along with an ugly focus ring for mouse clicks, as I just realized.

> Seems like a more appropriate question would be "why do we have a URL bar icon for blocked popups if
> we show a notification bar as well" (or why do we have a notification bar... etc.).

Why we have the notification bar seems obvious to me: it's more noticeable, which seems important in case we block a popup the user actually needs to see. I'm not sure why we have the URL bar icon, so this seems like an interesting question to me, but one that's much older and can be answered separately from whether this bug has merit.

Comment 29

[:Margaret Leibovic]

(In reply to Dão Gottwald [:dao] from comment #28)
> (In reply to :Gijs Kruitbosch from comment #27)
> > (In reply to Dão Gottwald [:dao] from comment #26)
> > > Why does this patch set padding:0 on the button? There's supposed to be room
> > > between multiple urlbar icons, hence this code:
> > > http://hg.mozilla.org/mozilla-central/annotate/56492f7244a9/browser/themes/
> > > linux/browser.css#l913
> > 
> > Please file a separate bug about this, because if there needs to be a change
> > there, we'll need to do the same for the reader-mode button. I expect we
> > can't use the default padding for a toolbar button here, though.
> 
> The reader mode button doesn't even use the urlbar-icon class. Margaret, can
> you take care of this?

Sure, I can file a bug to restore the urlbar-icon class on the reader mode toolbar button. I originally did do that in my first implementation:
https://hg.mozilla.org/mozilla-central/rev/a5db16049be1

But we decided to remove it here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1131458#c9

I wasn't familiar with the intention of this class, but it didn't seem very useful. Is this idea that add-ons will use this class as well?

> > > Also, why do we need this button to be keyboard accessible? We show a
> > > full-fledged notification bar for blocked popups.
> > 
> > Parity with the other icons?
> 
> This is misleading, since those icons have been images until recently. Most
> icons provided by add-ons are likely images still. So "the other icons"
> really only means the reader mode button.
> 
> The expectation so far has been that there's some other nicely accessible
> way provided, e.g. a menu item, a notification bar, etc.. It looks like the
> reader mode button breaks this (I would have expected a menu item under the
> View menu), but that doesn't seem like a good reason to go ahead with this
> bug.

We could file a bug about this. I imagine we were just following the UX that we implemented on Android without thinking through how this control would differ on desktop.

Comment 30

[Dão Gottwald [:dao]]

(In reply to :Margaret Leibovic from comment #29)
> Sure, I can file a bug to restore the urlbar-icon class on the reader mode
> toolbar button. I originally did do that in my first implementation:
> https://hg.mozilla.org/mozilla-central/rev/a5db16049be1
> 
> But we decided to remove it here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1131458#c9
> 
> I wasn't familiar with the intention of this class, but it didn't seem very
> useful. Is this idea that add-ons will use this class as well?

Yes, add-ons are expected to use this too such that all those icons are laid out and styled consistently.

Filed bug 1140340 and bug 1140345. I'll also back this one out if there are no further objections.

Comment 31

[Dão Gottwald [:dao]]

Backed out: https://hg.mozilla.org/integration/fx-team/rev/4fb7b71b56d4

Comment 32

[Dão Gottwald [:dao]]

Looks like this got merged: https://hg.mozilla.org/mozilla-central/rev/4fb7b71b56d4

Comment 33

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #28)
> The expectation so far has been that there's some other nicely accessible
> way provided, e.g. a menu item, a notification bar, etc.. It looks like the
> reader mode button breaks this (I would have expected a menu item under the
> View menu), but that doesn't seem like a good reason to go ahead with this
> bug.

From an a11y perspective, it's better if the same path can be used by users of AT as can be used by non-AT users. We've traditionally not made toolbar buttons accessible, but that tradition is changing (cf. MS Office ribbons).

> I suggest that we back this out and wontfix the bug, since its gain is
> highly questionable, it adds code complexity and introduces the
> aforementioned styling issue along with an ugly focus ring for mouse clicks,
> as I just realized.

So, this is an odd reasoning, because...

> > Seems like a more appropriate question would be "why do we have a URL bar icon for blocked popups if
> > we show a notification bar as well" (or why do we have a notification bar... etc.).
> 
> Why we have the notification bar seems obvious to me: it's more noticeable,
> which seems important in case we block a popup the user actually needs to
> see. I'm not sure why we have the URL bar icon, so this seems like an
> interesting question to me, but one that's much older and can be answered
> separately from whether this bug has merit.

... you actually reviewed this change, and it was added because you can disable the notification bar.

I don't understand why you think the icon *shouldn't* be accessible, when you thought it was necessary to add the icon despite the notification bar.

Comment 34

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #33)
> > > Seems like a more appropriate question would be "why do we have a URL bar icon for blocked popups if
> > > we show a notification bar as well" (or why do we have a notification bar... etc.).
> > 
> > Why we have the notification bar seems obvious to me: it's more noticeable,
> > which seems important in case we block a popup the user actually needs to
> > see. I'm not sure why we have the URL bar icon, so this seems like an
> > interesting question to me, but one that's much older and can be answered
> > separately from whether this bug has merit.
> 
> ... you actually reviewed this change, and it was added because you can
> disable the notification bar.
> 
> I don't understand why you think the icon *shouldn't* be accessible, when
> you thought it was necessary to add the icon despite the notification bar.

Ah, yes, I reviewed this. We were seeking parity with the status bar and that's what we got (in bug 618437 for those who are curious). Considering the big picture, whether a second indicator is a good idea is totally a valid question, just not at that time when we were trying to ship Firefox 4. I'm not sure that there's much value in allowing users to permanently disable the notification, so my gut feeling is that we should remove that option. Maybe we should get telemetry data on its usage first.