1696628 - Reader Mode indicator and add-on icons are stretched in the address bar

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Caspy7]

Attachment: image.png

According to s_hentzschel in the latest autoland build the reader mode indicator button appearance has regressed (it looks too tall).

Comment 1

[Marco Bonardo [:mak]]

It may be my regression, looking into it

Comment 3

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: visual regression

Comment 4

[Worcester12345]

No version specified, but for the record, not seeing it in Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:87.0) Gecko/20100101 Firefox/87.0 ID:20210304190020

Comment 6

[erosman]

Attachment: pageAction.jpg

Stretched PageAction icons

Comment 7

[Marco Bonardo [:mak]]

The blocking is fine for our tracking purposes.

Comment 15

[Marco Bonardo [:mak]]

Attachment: Bug 1696628 - Reader Mode indicator and add-on icons are stretched in the address bar. r=harry

The regression is actually caused by having changed urlbar-icon padding to a padding-inline, that was actually
an oversight due to the padding being excessive, with a 1px transparent border, we only need a 1 px padding.

This patch also tries to unify the padding management across urlbar icons and identity icons, because otherwise
in compact mode some icons padding gets unusually large, and they don't grow enough in touch mode.
Once Proton will be default this will allow simpler management of the paddings and urlbar sizes.

Comment 16

[Shane Hughes [:aminomancer]]

@:mak but also it depends on the image, if the icon width is not set explicitly, some of the images get squashed a little bit depending on the window scaling. and setting 1.5px padding as a general rule doesn't seem wise, since it's rendered differently depending on the window scaling too. this doesn't look right for me at 150% on windows 10 for example.

I know this is a bit of a major consideration but since these icons are functionally identical to toolbarbuttons, I think they should just be toolbarbuttons. they all need to have consistent padding, and some of them already have step animations, so why not make all of them toolbarbuttons? then you're not stuck sizing the image with padding properties, and don't need as many rules for density mode.

I guess if the image was set by "background-image" instead then you could use "background-size" to size it, but it seems more consistent to just make them toolbarbuttons, set the padding on the <toolbarbutton> element and set the width and height of the image on the .toolbarbutton-icon element.

Comment 17

[Shane Hughes [:aminomancer]]

sorry I meant to say if the icon viewBox is not set explicitly. for SVG images. I showed an example in the bug I reported:
more button:
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> <path.../> </svg>
reader button:
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"> <path.../> </svg>

I also went ahead and tested using background-image instead and it also squashes the images a little with my window scaling settings. that is, only squashes raster images and SVG images that don't have a viewBox set like the reader icon. but converting them to toolbarbuttons seems to fix everything

Comment 18

[Marco Bonardo [:mak]]

the icons should all appear as 16x16, and should be produced accordingly.

Comment 19

[Marco Bonardo [:mak]]

(In reply to aminomancer from comment #16)

@:mak but also it depends on the image, if the icon width is not set explicitly, some of the images get squashed a little bit depending on the window scaling. and setting 1.5px padding as a general rule doesn't seem wise, since it's rendered differently depending on the window scaling too. this doesn't look right for me at 150% on windows 10 for example.

I'm at 125%, it was not exactly right for me too, but that's what the spec says. This is more visible because there is a bug where the urlbar perceived height is wrong, it should be 32px, it is 30px instead. This patch fixes that. I can't exclude there may be later time tweaks after the ux review or during the Nightly cycle.

Comment 20

[Marco Bonardo [:mak]]

also note my comment only applies to proton, the current theme doesn't have padding.

Comment 21

[Shane Hughes [:aminomancer]]

yeah I have all the proton prefs enabled. and I know the urlbar and container are messed up internally but my own stylesheet overrides that, so I think this is a separate issue. try putting viewBox="0 0 16 16" in readerMode.svg, etc. many icons have viewBox attributes, many don't. I looked at dozens of them when I noticed this. obviously adding viewBox to the icons is not a solution since raster images from extensions won't have that. but it illustrates the problem with single <image> elements. that lack of viewBox is not a problem when those icons are used in other places in the UI, but since these are single <image> elements, the appearance of the image is going to depend on 1) the viewBox, 2) the padding. whereas if they were <toolbarbutton> elements, they'd get a container and you could set a width and height on the child .toolbarbutton-icon

Comment 22

[Shane Hughes [:aminomancer]]

as far as I can tell, that's the reason readerMode.svg looks all stretched and more.svg doesn't. although I think viewBox was omitted from readerMode.svg on purpose, since it's not a square icon. the actual viewBox could be generated in illustrator or inkscape but just for sake of argument, if you try "0 0 16 16" you can see that it immediately changes the size of the image regardless of padding-block

Comment 23

[Shane Hughes [:aminomancer]]

what I think would fix this completely and effortlessly is to just change browser.xhtml, line 2007 and so on, replace image with toolbarbutton. then, in browser-pageActions.js replace image with toolbarbutton as well

Comment 24

[Marco Bonardo [:mak]]

(In reply to aminomancer from comment #22)

as far as I can tell, that's the reason readerMode.svg looks all stretched and more.svg doesn't.

the reason is that I changed a padding into a padding-inline, nothing to do with the icon.

Comment 25

[Shane Hughes [:aminomancer]]

no, you changed padding into padding-inline for all the page action buttons. they ALL have 0 padding-block. why does this stretch readerMode.svg but doesn't stretch more.svg? because more.svg has viewBox="0 0 16 16" and readerMode.svg does not. readerMode.svg has no vertical bounds and no vertical padding. more.svg has no vertical padding either but it has vertical bounds, so it's constrained to a roughly square area. you can literally open the dev tools, go to any page action button, and uncheck the "padding-inline" rule and you'll see that the other page action buttons get scaled proportionally, while readerMode.svg will alternate from being stretched vertically to being completely square, because it has no viewBox attribute.

Comment 26

[Marco Bonardo [:mak]]

Sure, but what you are suggesting, to change into toolbarbutton can have deeper implications, it's not trivial at all. The kind of fix we want here is to restore the status-quo as far as possible. Those other things can be done and evaluated in the future when we're not time constrained.

Comment 27

[Shane Hughes [:aminomancer]]

Seems to work fine for me in browser-pageActions.js.

_makeUrlbarButtonNode(action) {
    let buttonNode = document.createXULElement("toolbarbutton");
    buttonNode.classList.add("urlbar-icon", "urlbar-page-action");
    buttonNode.setAttribute("actionid", action.id);
    buttonNode.setAttribute("role", "button");
    let commandHandler = (event) => {
        this.doCommandForAction(action, event, buttonNode);
    };
    buttonNode.addEventListener("click", commandHandler);
    buttonNode.addEventListener("keypress", commandHandler);
    return buttonNode;
}

needs a couple CSS rules though.

toolbarbutton.urlbar-icon {
    appearance: none;
}

toolbarbutton.urlbar-icon > .toolbarbutton-icon {
    width: 16px;
    height: auto;
}

actually it would be cleaner to define a new custom element so that <toolbarbutton> is given class "urlbar-icon-wrapper" and .toolbarbutton-icon is given class "urlbar-icon", since then we wouldn't need any CSS rules. but this way, changing just the tag name, we can at least see that nothing breaks. I'd test changing the built-in icons in browser.xhtml too, but it's a lot of work to rebuild and I figure that's even less likely to break something than changing the function

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c8b208e3863
Reader Mode indicator and add-on icons are stretched in the address bar. r=harry,desktop-theme-reviewers

Comment 29

[Natalia Csoregi [:nataliaCs]]

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

Comment 31

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

We verified this issue using Fx 88.0b5 on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 LTS.