Closed Bug 1696628 Opened 4 months ago Closed 4 months ago

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

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
88 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 --- unaffected
firefox88 --- verified

People

(Reporter: caspy77, Assigned: mak)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [proton-address-bar])

Attachments

(3 files)

Attached image image.png

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

It may be my regression, looking into it

Assignee: nobody → mak
Status: NEW → ASSIGNED
Whiteboard: [proton-address-bar]
Duplicate of this bug: 1696648

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

Severity: -- → S3
Keywords: regression
Priority: -- → P1
Summary: Proton: Reader Mode indicator is too tall (regression) → Reader Mode indicator and add-on icons are too tall in the address bar

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

Duplicate of this bug: 1696649
Summary: Reader Mode indicator and add-on icons are too tall in the address bar → Reader Mode indicator and add-on icons are too stretched in the address bar
Summary: Reader Mode indicator and add-on icons are too stretched in the address bar → Reader Mode indicator and add-on icons are stretched in the address bar
Attached image pageAction.jpg

Stretched PageAction icons

No longer blocks: proton-address-bar

The blocking is fine for our tracking purposes.

Duplicate of this bug: 1696672
Regressed by: 1691545
Duplicate of this bug: 1696683
Duplicate of this bug: 1696657
Duplicate of this bug: 1696697
Duplicate of this bug: 1696703
Duplicate of this bug: 1696724
Duplicate of this bug: 1696743

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.

Blocks: 1694105

@: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.

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

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

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

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

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

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

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

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

Blocks: 1695022

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.

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.

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

Pushed by mak77@bonardo.net:
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
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Duplicate of this bug: 1696864
Regressions: 1697801
See Also: → 1698986
Flags: qe-verify+

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.