1157123 - The bottom border of the Serif button displays an escaping pixel at its left side

Status: RESOLVED WORKSFORME

(Toolkit :: Reader Mode, defect, P5)

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot

Note: this is a follow-up issue for Bug 1149068.

Context: this issue was initially fixed in Bug 1142298 and confirmed fixed on Nightly 39.0a1 (2015-03-20).

Reproducible on:
Nightly 40.0a1 (2015-04-21), Aurora 39.0a2 (2015-04-20), Beta 38.0b6-build1 (20150420134330)

Affected platforms:
Windows 7 (x64), Windows 8.1 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5

Steps to reproduce:
1. Launch Firefox.
2. Open a Reader View-compatible page, e.g. http://www.nytimes.com/2015/04/22/world/asia/india-seeking-a-boost-plans-to-put-its-idle-gold-to-work.html
3. Click the "Enter Reader View" button from the Location Bar.
4. Open the "Type controls" by clicking the "[Aa]" button.
5. Click the "Serif" button and zoom in until about 200%.

Expected result:
The UI of the Type controls panel is displayed properly and in accordance with the design specifications.

Actual result:
The bottom border of the "Serif" button is showing an additional pixel at its left side.

Regression range:
— mozilla-central:
* Last good revision: 8af276ab8636 (2015-03-31)
* First bad revision: 37ddc5e2eb72 (2015-04-01)
* Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8af276ab8636&tochange=37ddc5e2eb72
— mozilla-inbound:
* Last good revision: ab01fd91bc02
* First bad revision: 870046adb53a
* Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ab01fd91bc02&tochange=870046adb53a
— potential regressor:
https://hg.mozilla.org/integration/mozilla-inbound/rev/049a2ddcc904 — Blake Winton — Bug 1148050 - Make Reader View type panel look closer to the spec. ui-r=mmaslaney, r=Unfocused

Comment 1

[Anson]

Hi, I'm relatively new but I would like to attempt to fix this bug.

Comment 2

[Anson]

A couple thoughts from my brief inspection:

I believe this is the file of interest

https://searchfox.org/mozilla-central/source/toolkit/themes/shared/aboutReader.css

Lines 411 and 416 describe the orange highlight line that appears for the button you select.

So for both color-scheme and font-type, you get an orange box-shadow, while for font-type, in addition to the box-shadow, it makes the border-bottom orange as well (in an attempt to hide the grey border line below). The problem is because CSS borders are mitered, so we get a weird 45 degree corner that spills into the middle grey line (and looks like an out-of-place orange pixel).

Simply removing lines 414-417 will avoid this issue, but leave the grey border line visible below the orange highlight. I can see that is slightly jarring, and undesirable.

As for how we can color the bottom border orange without the mitered corner I do not know right now, maybe we need some CSS workaround to avoid this.

Comment 3

[:Gijs (he/him)]

(In reply to Anson from comment #2)

<snip>
The problem is because CSS borders are mitered, so we get a weird 45 degree corner that spills into the middle grey line (and looks like an out-of-place orange pixel).

Yep, this diagnosis is correct, I believe.

Simply removing lines 414-417 will avoid this issue, but leave the grey border line visible below the orange highlight. I can see that is slightly jarring, and undesirable.

Can you post screenshots of what you mean? I'm actually not sure. I thought the box-shadow was on top of the borders anyway, but I'm probably wrong, I didn't look at this very long the last time I poked about.

In principle, you can define multiple (inset) box-shadows (comma-separated, I think), which might work better than the border here? Does that sound feasible?

Comment 4

[Anson]

Attachment: box-shadow-example

Comment 5

[Anson]

In the following screenshot you can see the result after removing lines 414-417

https://bugzilla.mozilla.org/attachment.cgi?id=9049577

Basically there is a 3px high orange box-shadow above the bottom grey border. By slightly jarring, I meant the orange highlight line is immediately followed by a 1px grey border below it.

Previously (the first screenshot attached to this bug), in addition to this box-shadow, we would also color this bottom grey border orange, thus we would essentially have a 4px high orange highlight line, with no grey line.

Correct me if I'm wrong, but I don't believe you can cover the border with a box-shadow, its either above the border line if using inset, or below the border line if using outset (slightly simplifying here).

Comment 6

[:Gijs (he/him)]

(In reply to Anson from comment #5)

Thanks, the screenshots help.

Correct me if I'm wrong, but I don't believe you can cover the border with a box-shadow, its either above the border line if using inset, or below the border line if using outset (slightly simplifying here).

You're absolutely correct. However, you can simulate a border with a box-shadow. So you could remove the border, and instead have a 1px inset grey box-shadow with no blur/spread/whatever, that matches the border, and then in the "selected" case, override the thin grey box-shadow with the thicker orange one.

Hacky, of course, but seems to me like it'd work? Or is there some other reason that I haven't thought of that means it doesn't?

Comment 7

[:Gijs (he/him)]

Hey Anson, are you still interested in working on this?

Comment 8

[Anson]

Sorry, been swamped with school so I haven't been working on this.

If anyone else is interested feel free to work on this, otherwise I will take a look again in a week or so when my schedule opens up.

Comment 9

[Monika Maheshwari [:MonikaMaheshwari]]

Hey, I can work on this issue.

Comment 10

[:Gijs (he/him)]

(In reply to Monika Maheshwari [:MonikaMaheshwari] from comment #9)

Hey, I can work on this issue.

Hey Monika, I noticed you've fixed a number of good-first-bug / mentored issues already - how about something slightly harder, like bug 1450924?

Comment 11

[:Gijs (he/him)]

This got fixed when we changed the reader mode design.