1130609 - Scroll urlbar (and toolbar) with the page

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Wesley Johnston (:wesj)]

We should scroll the urlbar and the titlebar with the page, like we do on Android.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Pull request

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

On iPad we don't show the toolbar at the bottom but it is combined with the url bar instead. How about we rename this bug so that this covers the iPhone case and then do a similar bug to introduce the iPad url/toolbar?

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I think this code is ok but I have one design choice question.

Instead of letting urlbar and toolbar have a background view and doing a transform on that background view ... why don't we just change the position of the whole urlbar and toolbar? They are anchored to the top and bottom of the screen with a constraint so to make them move up/down the only thing we need to do is move the contraint's constant to change the offset.

I think if the BrowserViewController manages the position of urlbar and toolbar (it is the view *controller*) after all, the code would be much simpler. The view hierarchies of the urlbar and toolbar can be simpler and they don't have to contain any logic about their placement. (which should not be their concern)

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8560718 [details] [review]
Pull request

I'm playing with this and I just noticed that when the top and bottom bar have moved off screen, you cannot tap those areas anymore.

This is a side effect of keeping those views on screen while moving their contents.

I really want to suggest to change this patch to move the position of the whole views instead of only their contents.

Comment 5

[Wesley Johnston (:wesj)]

(In reply to Stefan Arentz [:st3fan] from comment #3)
> I think if the BrowserViewController manages the position of urlbar and
> toolbar (it is the view *controller*) after all, the code would be much
> simpler. The view hierarchies of the urlbar and toolbar can be simpler and
> they don't have to contain any logic about their placement. (which should
> not be their concern)

Hmm.. I wanted them to be aware only because some of the mockups show a "small" urlbar at the top of the screen, which I assume is like the Safari shrunken urlbar. I assumed the view itself would handle that transition, and the view controller would be unaware of its appearance.

But one thing that bothered me about this approach is that the two toolbars are not kept in sync in any way, so maybe this will make that feel a bit better. I'll try it.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I think you need to do two different things then:

The top bar does not slide off screen, instead it just resizes, while being anchored against the top. When you resize the view, it's layoutSubviews will be called repeatedly. That could be a good place to slowly change the contents based on it's shrunken size. You could for example fade things out and in based on the percentage it has shrunk.

The bottom bar has no special effects I think? So it can just move off screen.

Btw be sure to test while you have the webview zoomed in. I saw some really strange things happening, like both bars moving to the left/right.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Pull request

Updated. The view controller handles all the scrolling now. I couldn't get constraints to work, so I'm still using transforms right now.

The clicking issues you saw were because we have contentInsets set on the webview. I add/remove them as you scroll now (necessary to make absolute positioned elements work as well...)

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8563056 [details] [review]
Pull request

Looks good to me.

Just two nits:

there is something weird going on when you are at the bottom of the page, but we can do that in a followup fix

the PR also contains some other changes, i think related to bookmarks or sharing. maybe a good idea to move those to a separate pr?

Comment 9

[Wesley Johnston (:wesj)]

Yeah. Deleted the other changes. I also saw some of the problems you saw. They're related to us changing the content insets, which changes the scroll position of the page as we're moving. I also saw methods getting called concurrently sometimes. I've fixed most of them, but still managed to make the urlbar "panic" very occasionally. I'm fine fixing the rest in product.