1525805 - The floating text icon in reader mode scrolls away

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

The floating text settings icon in reader mode is fixed position and the fixed position implementation had changed. So that will probably need reimplementation.

Comment 1

[Jan Henning [:JanH]]

Doesn't seem to happen on every page, so here is one where this is observable for me:
https://www.hq.nasa.gov/office/pao/History/SP-4204/ch1-3.html

Comment 2

[Jan Henning [:JanH]]

Looking at bug 1520077, would this issue be fixed by bug 1516377, too?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I don't think so. I think bug 1516377 makes this issue more worse. The site in comment 1 overflows on my Android phone (Sony D5803) because some images are wider than ICB and the reader mode specifies 'overflow:hidden' there? If so, the icon is attached at the right bottom of the layout viewport by bug 1516377, so it might not be in view. Bug 1519013 prevents the overflow in the case of this particular site, but even so there might be other sites that affected by 1520077.

Keep NI to me for now.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

The style for the icon is specified by this '.toolbar' class? Though I don't quite understand how the icon is positioned at the right bottom of the display because the left:0 is specified (I thought right:0 is specified instead), it doesn't matter for now since I suppose expanding the layout viewport to the vertical direction rarely happens.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Anyway, the overflow is a real problem now, bug 1519013 should fix it. That's said, I am not sure it is desirable for the reader mode.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Gosh! I did totally misunderstand this bug. The icon will be mis-placed by bug 1516377, it will only be seen at the bottom of the content.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Just using position:stick instead of position:fixed fixes the issue?

Comment 8

[Jan Henning [:JanH]]

If the layout viewport is expected to remain as it is now (i.e. possibly larger than the screen size), we'd need to fix bug 1525967 as well then.

Comment 9

[Botond Ballo [:botond]]

The layout viewport should be equal in size to the visual viewport when you're zoomed out as far as you can.

On the page from comment 1, you can't zoom out, so I would expect the two viewports to match in size, and therefore fixed-position elements to remain fixed to the visual viewport.

It would be good to understand why that's not happening.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #9)

The layout viewport should be equal in size to the visual viewport when you're zoomed out as far as you can.

On the page from comment 1, you can't zoom out, so I would expect the two viewports to match in size, and therefore fixed-position elements to remain fixed to the visual viewport.

It would be good to understand why that's not happening.

I am not sure this is the answer to your question. The reader mode (or the site) specifies user-scalable=no and the site is not automatically scaled down because, I guess, loading the wider images are generally done after the initial paint. Anyway, I've confirmed that bug 1519013 fixes the overflow issue on the site.

Or you mean in the case of user-scalable=no we should disallow expanding the layout viewport?

Comment 11

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)

Or you mean in the case of user-scalable=no we should disallow expanding the layout viewport?

We should check Chrome's behaviour, but intuitively, I would think that user-scalable=no would play a role in determining the "mininum scale" used to calculate the "minimum scale size". That is, on a page with user-scalable=no, I would expect the "minimum scale" to be the scale at which the page is loaded (and which the user cannot then change).

Comment 12

[Jan Henning [:JanH]]

Don't forget that we optionally allow overriding user-scalable=no, so we still need to behave in a sane way if the user scales the page anyway (or use the effective user-scalable value, taking into account the user override).

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 67, if this is not fixed during the nightly cycle, I'll accept an uplifted patch early in the beta cycle.

Comment 14

[Marcia Knous [:marcia]]

P2 as discussed during triage.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Minimum-scale with user-scalable=no

(In reply to Botond Ballo [:botond] from comment #11)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)

Or you mean in the case of user-scalable=no we should disallow expanding the layout viewport?

We should check Chrome's behaviour, but intuitively, I would think that user-scalable=no would play a role in determining the "mininum scale" used to calculate the "minimum scale size". That is, on a page with user-scalable=no, I would expect the "minimum scale" to be the scale at which the page is loaded (and which the user cannot then change).

You are totally right. I should have checked Chrome's behavior first.

Surprisingly, Chrome doesn't allow using the minimum scale size for the content having user-scalable=no. (I did also expect the page disallow scaling after the minimum scale is applied).

Anyway, to fix this reader mode issue, we can simply skip applying the minimum scale for user-scalable=no contents.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Oops, I did forget about JanH's comment. We also need to change aboutReader.css in the case where the user changes the setting (but hopefully it can be done in a later bug).

Comment 17

[Botond Ballo [:botond]]

Bug 1525948 should improve the behaviour here, because it keeps the layout viewport's aspect ratio locked to that of the visual viewport. So, if the article content isn't wider than the screen, the layout viewport height won't be taller, either.

For a full solution, we should still factor user-scalable=no into the minimum scale size.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Huh? I thought I did push the fix to Phabricator last week..

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1525805 - Don't apply the minimum scale size if user-scalable=no is specified. r=botond

Comment 20

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bfd997c08e524c69a7dbf92e6e3f47d5d89c948

I am not sure whether an additional reftest for the case where initial-scale is specified works on Windows or not.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/931ffb6a2a7a
Don't apply the minimum scale size if user-scalable=no is specified. r=botond

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b7598cd4319
Backed out changeset 931ffb6a2a7a by request. CLOSED TREE

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75260b774b5c
Don't apply the minimum scale size if user-scalable=no is specified. r=botond

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/75260b774b5c