943921 - Regression: Buttons on Twitter main splash page are partially obscured

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Beta | Aurora (Twitter screenshot comparison)

http://twitter.com

See screenshot comparison

--
Nightly|Aurora (11/27) | LG Nexus 4 (Android 4.4)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Could be related to some of the "top margin" bugs I've seen filed lately

Comment 2

[Aaron Train [:aaronmt]]

Don't think so. Aurora is affected.

Comment 3

[Teodora Vermesan (:TeoVermesan)]

Regression window-wanted:

mc:
not affected build: 02-10-2013
affected build: 03-10-2013  
-pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e3c84e9f2490&tochange=0e26e6f12ad9

Comment 4

[Aaron Train [:aaronmt]]

7e8f946d0d1a	Kartikaya Gupta — Bug 919437 - Use the scrollport size as the inner size if it is set. r=tn, roc

tn are you able to reproduce this this bug?

Comment 5

[Timothy Nikkel (:tnikkel)]

Yeah, I can reproduce this. It looks like they are sizing their page to innerHeight, and our innerHeight value does not factor in the dynamic toolbar. kats, do you think we should revisit that decision based on this?

Comment 6

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

For the record, this was discussed in bug 923663 which I wontfix'ed.

I also checked to see what the behaviour on twitter.com is and although the buttons are cut off in the initial view, the page can be scrolled to hide the dynamic toolbar, in which case the page looks fine. This agrees with the conceptual model I described in bug 923663 comment 12. I think Chris should probably weigh in here as well.

If we decide to keep the current FF behaviour then we can move this to Tech Evangelism and ask Twitter to not use innerHeight unless they want to keep the current user experience - they should be able to use height=device-height or some other such mechanism to size the page to fit with the dynamic toolbar visible.

Comment 7

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> For the record, this was discussed in bug 923663 which I wontfix'ed.
> 
> I also checked to see what the behaviour on twitter.com is and although the
> buttons are cut off in the initial view, the page can be scrolled to hide
> the dynamic toolbar, in which case the page looks fine. This agrees with the
> conceptual model I described in bug 923663 comment 12. I think Chris should
> probably weigh in here as well.
> 
> If we decide to keep the current FF behaviour then we can move this to Tech
> Evangelism and ask Twitter to not use innerHeight unless they want to keep
> the current user experience - they should be able to use
> height=device-height or some other such mechanism to size the page to fit
> with the dynamic toolbar visible.

This is probably not a useful thing to be saying, but I agree with the reporter in bug 923663 - I think when we resize the CSS viewport to not include the dynamic toolbar in browser.js and measure, the innerHeight should be smaller. Otherwise, this is going to break a lot of pages, and like the reporter mentions, it doesn't match behaviour in Chrome (or, I believe, Safari).

Comment 8

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

What about during the animation? And should we be firing resize events to content when the toolbar shows/hides?

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> What about during the animation? And should we be firing resize events to
> content when the toolbar shows/hides?

We only set the CSS viewport at two sizes - screen size, or screen-minus toolbar, during measurement. If we determine the page to fit into the screen minus toolbar, then that's the size of the window, otherwise it's the size of the whole window. I think I may be misunderstanding what's in question here...? If the toolbar is in a state where it can animate, it means the page can't fit into the screen minus toolbar, so in that case, the window size is unaffected by the toolbar visibility.

Basically, toolbar visibility never affects window size, but page size does.

Comment 10

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

As of bug 919437, the innerWidth/innerHeight return the "visual viewport" rather than the "layout viewport" (aka the CSS viewport). So really when we set the CSS viewport is irrelevant; what we care about is how large the user-visible content area is.

While the toolbar animates off screen, that area changes, and so I would expect innerHeight to also follow along. Content might depend on this if they're doing something wacky like simulating position:fixed with JS (listening for resize events and moving content accordingly). If we don't animate the innerHeight to be in sync with the toolbar animation then the faux-position:fixed would jump. I guess it's probably not important enough of a use case but if we're going to change this I just want to make sure I understand what our expected behaviour is.

Comment 12

[Benjamin Kerensa [:bkerensa]]

Triage Comment:
We are going to wait on tracking this until we have more information as to whether this is a bug needing tracking or something that will go to Tech Evangelism.

Comment 13

[Benjamin Kerensa [:bkerensa]]

Per Comment 6 are we keeping the behavior or do we want to move this to Tech Evangelism

Comment 14

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

We can change our behaviour if Cwiiis is ok with the behaviour I'm describing in comment 10.

Comment 15

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> As of bug 919437, the innerWidth/innerHeight return the "visual viewport"
> rather than the "layout viewport" (aka the CSS viewport). So really when we
> set the CSS viewport is irrelevant; what we care about is how large the
> user-visible content area is.
> 
> While the toolbar animates off screen, that area changes, and so I would
> expect innerHeight to also follow along. Content might depend on this if
> they're doing something wacky like simulating position:fixed with JS
> (listening for resize events and moving content accordingly). If we don't
> animate the innerHeight to be in sync with the toolbar animation then the
> faux-position:fixed would jump. I guess it's probably not important enough
> of a use case but if we're going to change this I just want to make sure I
> understand what our expected behaviour is.

I'm in two minds about this, which isn't helpful I understand.

On the one hand, I think that the margins should only be taken into account in innerWidth/innerHeight if they're permanently visible (pinned). I say this, as I think it may be a security concern if content can detect when the toolbar is/isn't visible, which it currently can't in any useful/exploitable way.

On the other hand, absolutely positioned and JS-positioned content may end up shoved off the bottom of the screen if we don't update innerWidth/innerHeight on pages that cause an unpinned toolbar. I don't like the relayouting implications for though, performance is very likely to be impacted badly if we change innerWidth/Height on toolbar visibility.

It's telling though, that the only use cases for this size to change with the toolbar are things that we really shouldn't be encouraging web developers to do (absolute positioning relative to the bottom of the screen on pages that are larger than the screen and simulating position:fixed).

Taking all this into account, I'm going with my first paragraph - innerWidth/innerHeight should only take the toolbar into account if it's pinned, otherwise they should reflect the full viewable area including the toolbar, regardless of the toolbar visibility.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Does that mean this is a WONTFIX ?

Comment 17

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

Yes. Well, it's either a WONTFIX or this bug should be moved to Tech Evangelism.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Will track for now, even if it's Tech Evangelism it would still be good to take a stab at requesting change since this is such a high-traffic site and visually that change is jarring.

Comment 19

[Aaron Train [:aaronmt]]

Like magic I don't even see this anymore ....

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

No longer tracking in that case, renom if it is REOPENED.