1978145 - Implement dynamic toolbar on top in RDM

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, task, P2)

Description

[Alex Jakobi [:ajakobi]]

The initial implementation of the dynamic toolbar in responsive design mode only shows the toolbar at the bottom.
This task should enable positioning the toolbar at the top.

Comment 2

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1978145 - Remove resizer offset and browser border.

This makes it easier to line things up accurately.

Comment 3

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1978145 - Add rudimentary toolbar-on-top support.

Comment 4

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1978145 - Add a browser bottom cover to hide the part that's pushed offscreen when the top toolbar is visible.

Comment 5

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1978145 - Move mouse motion detection to the parent, and add snap animation when the mouse button is lifted.

The current contentScrolled message doesn't let us know when the mouse button is released.

I realized that we don't necessarily need tho dynamic toolbar mouse tracking to be done by the touch simulator.
We can just listen to mouse events in two different places.

This patch makes us no longer use the contentScrolled message. We can remove it again.

Comment 6

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1978145 - Improve the visuals of the RDM dynamic toolbar.

Comment 7

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1978145 - Add an .rdm-screen-box element which, unlike the browser, doesn't move, and make it render the shadow. Also bring back the border as another shadow.

Comment 8

[Alexandre Poirot [:ochameau]]

Alex, Markus, Any way DevTools team can help with this ongoing work?

This looks like a very useful feature to help WebCompat team investigate mobile issues from desktop.

Comment 9

[Markus Stange [:mstange]]

Hi Alex, sorry for the delay. It would be great if you could take these patches and drive them to completion!

I think the code itself is fine, it's more that the patch series isn't fully optimized for ease of review.

If I remember correctly, the things left to do were:

• Merge and re-split the patches so that the visual look is maintained at any point in the patch stack. Specifically, there's one part which gets rid of the borders and shadows, and another part which adds a new element with the shadow to restore the original look, so I think it would make sense to move this "solution" to the start of the stack.
• Split the "Move mouse motion detection to the parent, and add snap animation when the mouse button is lifted." patch into the two pieces described by its commit message
• Do another review pass of the patches and maybe split out things that don't really belong in a patch. For example I see that D265176 is also centralizing the pref reading, and it's moving the initialization of the dynamic toolbar behavior down so that it's not interspersed between the DOM element creation that the rest of the function is doing. So maybe those things would be better in a separate patch.

Comment 10

[Markus Stange [:mstange]]

I've picked this back up.

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Make RDM element sizes and positions easier to reason about. r=#devtools-reviewers

Remove the --resizer-offset variable (which really meant "reserved space for side
resize handles") and replace the browser border with an outline, so that at zoom:1
the browser size exactly matches rdm-width x rdm-height. We use negative margins on
the side resize handles to center their interactable location on the respective
side border.

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Simplify RDM resizer positioning in the presence of zoom, by introducing a scaled wrapper element. r=#devtools-reviewers

This makes the dynamic toolbar have the correct size when using page zoom in RDM.

There's one user-visible change that this introduces: The size of the resizer, as well
as the size of the interactive areas of the side resize handles, now depends on the zoom.

Comment 13

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Use a .dynamic-toolbar-enabled class instead of .style.visibility. r=#devtools-reviewers

Just feels a bit cleaner and is more consistent with the .dynamic-toolbar-on-top
class which I'll be adding in a future patch.

Comment 14

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Make .rdm-dynamic-toolbar look more like an actual toolbar. r=#devtools-reviewers

This gives it a "URL bar" (just a shape with rounded endcaps) and a
"tab count indicator" (just a rounded rect with a 1 in it) just so
that it feels a bit more familiar.

Comment 15

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Increase DYNAMIC_TOOLBAR_MAX_HEIGHT to 50px, because a 50px toolbar looks more realistic. r=#devtools-reviewers

Comment 16

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Make the RDM dynamic toolbar snap to fully-visible or fully-hidden when the mouse is released. r=#devtools-reviewers

This also moves the mouse motion detection to the parent, and removes the use of the
contentScrolled message.

The problem with the contentScrolled message is that it doesn't let us know when the mouse button is released.
Rather than adding another message, I think it's simpler to just do mouse tracking in two places.
There's not much value in having the touch simulator (which runs inside the content) do the
scroll tracking.

This patch doesn't remove the code that fires the contentScrolled message.
That might be worth doing in a follow-up, but I'm inclined to leave it for now, in case we want to use
it again in the future.

Comment 17

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Add support for toolbar-on-top to RDM's dynamic toolbar mode. r=#devtools-reviewers

This is rather messy, because the approach of "toolbar pushes down browser"
matches the real implementation in Fenix which is also rather messy.

It works well enough to reproduce some formerly mobile-only bugs.

Having this functionality will be helpful during the work on the
"overlap model" (bug 1921331), and the result of that work should let us
simplify this RDM code again.

There's one known bug in this RDM implementation, which is that dragging the
page to scroll down can sometimes scroll the content at twice the desired speed
if the content is also moved by a growing / shrinking toolbar. The real Fenix
implementation does not have this bug. But it's not a big problem.

Comment 18

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=6bb8cf7e1e0397d4d03b5d0a1fe608a0d336ca94

Comment 19

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod&landoCommitID=193040

Comment 20

[Markus Stange [:mstange]]

Attachment: Bug 1978145 - Reset setVerticalClipping and setDynamicToolbarMaxHeight when exiting RDM. r=#devtools-reviewers

Unrelated to this bug, but something I noticed while using RDM: After
exiting RDM, fixed-to-bottom toolbars would sometimes be in the wrong
spot.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ad32300ffdbf
https://hg.mozilla.org/integration/autoland/rev/c7ebc7fdebec
Make RDM element sizes and positions easier to reason about. r=devtools-reviewers,nchevobbe
https://github.com/mozilla-firefox/firefox/commit/9a9555c4023f
https://hg.mozilla.org/integration/autoland/rev/c58dfb858319
Simplify RDM resizer positioning in the presence of zoom, by introducing a scaled wrapper element. r=devtools-reviewers,nchevobbe
https://github.com/mozilla-firefox/firefox/commit/e0c761fc081e
https://hg.mozilla.org/integration/autoland/rev/7b0cb42dc962
Use a .dynamic-toolbar-enabled class instead of .style.visibility. r=devtools-reviewers,nchevobbe
https://github.com/mozilla-firefox/firefox/commit/56abc6071736
https://hg.mozilla.org/integration/autoland/rev/5a5d73443044
Make .rdm-dynamic-toolbar look more like an actual toolbar. r=devtools-reviewers,nchevobbe
https://github.com/mozilla-firefox/firefox/commit/afed24ab1ee2
https://hg.mozilla.org/integration/autoland/rev/ade2d231c9a6
Increase DYNAMIC_TOOLBAR_MAX_HEIGHT to 50px, because a 50px toolbar looks more realistic. r=devtools-reviewers,nchevobbe
https://github.com/mozilla-firefox/firefox/commit/c5429f17ede6
https://hg.mozilla.org/integration/autoland/rev/c3ab11c4f10f
Make the RDM dynamic toolbar snap to fully-visible or fully-hidden when the mouse is released. r=devtools-reviewers,nchevobbe
https://github.com/mozilla-firefox/firefox/commit/56015cceeb39
https://hg.mozilla.org/integration/autoland/rev/a14aecb1b2f4
Add support for toolbar-on-top to RDM's dynamic toolbar mode. r=devtools-reviewers,nchevobbe
https://github.com/mozilla-firefox/firefox/commit/de98b4b039cf
https://hg.mozilla.org/integration/autoland/rev/ecb278dc62ea
Reset setVerticalClipping and setDynamicToolbarMaxHeight when exiting RDM. r=devtools-reviewers,nchevobbe

Comment 22

[rperta]

https://hg.mozilla.org/mozilla-central/rev/c7ebc7fdebec
https://hg.mozilla.org/mozilla-central/rev/c58dfb858319
https://hg.mozilla.org/mozilla-central/rev/7b0cb42dc962
https://hg.mozilla.org/mozilla-central/rev/5a5d73443044
https://hg.mozilla.org/mozilla-central/rev/ade2d231c9a6
https://hg.mozilla.org/mozilla-central/rev/c3ab11c4f10f
https://hg.mozilla.org/mozilla-central/rev/a14aecb1b2f4
https://hg.mozilla.org/mozilla-central/rev/ecb278dc62ea

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for the Fx152 relnotes?
https://wiki.mozilla.org/Release_Management/Release_Notes_Nomination#How_to_nominate_a_bug_for_release_notes_addition

Comment 24

[Markus Stange [:mstange]]

It's still behind a hidden pref. I think we can add it once we add an actual button / checkbox in the UI to enable it.