1226898 - Autoscroll breaks the markup view if the height of the view is less than 100px

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Sami Jaktholm]

STR:
1. Open Inspector
2. Reduce the height of the toolbox
3. Start dragging a node

What happens: The autoscroll tries to scroll the view in both directions and leaves intervals running that will keep scrolling the view until the browser is closed.

What should happen: The view should be scrolled to the direction that's closer to the pointer.

Comment 1

[Patrick Brosset <:pbro>]

Oh indeed, if the height of the toolbox is small enough, then the autoscroll mechanism gets confused (probably when the height is smaller than the twice the configured distance from the edge that triggers autoscroll:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#18 )

We should make this distance smaller I think, as a first step (which would require to move the mouse closer to the edge to start the auto-scroll), 50px seems too big to me now somehow.
But also, as the markupview becomes smaller, we should probably adapt this distance dynamically too. And make sure we don't have a sort of infinite loop problem where scrolling never ends.

Comment 2

[Greg Tatum [:gregtatum]]

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

Comment 3

[Greg Tatum [:gregtatum]]

Attachment: Autoscroll breaks the markup view if the height of the view is less than 100px

I made it so that the scroll maxes out at 50px, but shrinks to a ratio of
the element height when it's smaller.

Comment 4

[Greg Tatum [:gregtatum]]

Attachment: inspector-drag.mov

Video showing the new behavior.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8743650 [details] [diff] [review]
Autoscroll breaks the markup view if the height of the view is less than 100px

Review of attachment 8743650 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks for working on this.
I have a couple of comments I'd like to see addressed before I can R+ this:

- The commit message should describe the change, not the problem. So maybe something like "Make autoscroll distance depend on the document height"
- I'd like to see a test for this. We already have one here: \devtools\client\inspector\markup\test\browser_markup_dragdrop_autoscroll.js
It might need to be changed, and I think you should add a second one similar to this that opens a tiny toolbox and tries to autoscroll.

::: devtools/client/inspector/markup/markup.js
@@ +14,5 @@
>  const DEFAULT_MAX_CHILDREN = 100;
>  const COLLAPSE_DATA_URL_REGEX = /^data.+base64/;
>  const COLLAPSE_DATA_URL_LENGTH = 60;
>  const NEW_SELECTION_HIGHLIGHTER_TIMER = 1000;
> +const DRAG_DROP_AUTOSCROLL_EDGE_MAX_DISTANCE = 50;

Maybe reduce the max distance to something like 30?
50 means that for anything lower than 500px high, the edge distance will vary with the height. I'm unsure if this is a problem, but I tend to think that most people using the drag/drop feature would have a rather high toolbox, so something higher than 300px. So having a fixed value for anything higher than this would probably be good.

@@ +20,2 @@
>  const DRAG_DROP_MIN_AUTOSCROLL_SPEED = 5;
>  const DRAG_DROP_MAX_AUTOSCROLL_SPEED = 15;

I think our speed might be too fast. When the markup-view is very small, it's almost impossible to drag precisely enough to choose a slow speed and you almost always end up dragging all the way down or up. I think simply adjusting the min speed to be slower and the range between the min and max speed to be shorter would help.
I tried with:
const DRAG_DROP_MIN_AUTOSCROLL_SPEED = 2;
const DRAG_DROP_MAX_AUTOSCROLL_SPEED = 8;
and it seemed to work fine in all cases. Let me know what you think.

Comment 6

[Greg Tatum [:gregtatum]]

bgrins: I was chatting with pbro and he suggested I hit you up on an issue I came across on Mac. The scrollbar automatically appears and hides on scrolling DOM elements. These scrollbars overlay the element itself, but aren't measurable at all.

In the inspector, when you are dragging a DOM node, you end up mousing over this scrollbar which stops the mousemove event. This makes it quite difficult to autoscroll down with a small inspector window. I don't know of any clean way to measure the existence of these magical scrollbars and take them into account. The only way I can think is to detect the OS and put in some magic padding, which sounds really nasty. Do you have any thoughts on this?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #6)
> bgrins: I was chatting with pbro and he suggested I hit you up on an issue I
> came across on Mac. The scrollbar automatically appears and hides on
> scrolling DOM elements. These scrollbars overlay the element itself, but
> aren't measurable at all.
> 
> In the inspector, when you are dragging a DOM node, you end up mousing over
> this scrollbar which stops the mousemove event. This makes it quite
> difficult to autoscroll down with a small inspector window. I don't know of
> any clean way to measure the existence of these magical scrollbars and take
> them into account. The only way I can think is to detect the OS and put in
> some magic padding, which sounds really nasty. Do you have any thoughts on
> this?

I think it's something we might have to live with if mouse events are not firing - because those are native OS scrollbars.  At least, it's not something to block your fixes here on and we could have a follow up for further investigation.

Is this a horizontal scrollbar that's getting in the way?  Maybe we could change overflow-x to hidden while dragging.  If it's a vertical scrollbar, I wouldn't worry about it because you'd have to be in a super narrow and unusable markup view for that to be an issue.

A few general notes about these scrollbars that may or may not help here:
* We simulate these 'floating scrollbars' for the dark theme and in responsive design mode regardless of the platform (see https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/floating-scrollbars-dark-theme.css and https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/theme-switching.js).  I'm not sure if this will also be a problem in Windows dark theme, but it seems possible.
* You can disable this behavior on OSX which is helpful sometimes for testing various things - go to Preferences -> General -> Show Scroll Bars -> Always
* Maybe if we listen for mouse events at a higher level we could capture them even when they are over the scrollbar.

Comment 8

[Greg Tatum [:gregtatum]]

Attachment: Make autoscroll distance depend on the document height

The internal setInterval implementation changed, and the autoscrolling behavior now happens
super quickly in Nightly. I went ahead and fixed that in this patch by switching over to
the more predictable requestAnimationFrame. I addressed the overflow scrollbar issue per
bgrin's idea of hiding it on dragging. I also hit the dragging speeds. I made the speed
a little variable so that it slows down the smaller the window is. I felt like the
different sized windows needed different scroll speeds to feel natural.

The one thing I'm having trouble figuring out is how to use the browser test to control
the height of the element. There are a lot of helper functions that I'm having trouble
figuring out where they come from. I'd appreciate a little direction on how to safely
change the toolbox height. Right now it's a pretty small window when I test it on this
machine as it is.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8751929 [details] [diff] [review]
Make autoscroll distance depend on the document height

Review of attachment 8751929 [details] [diff] [review]:
-----------------------------------------------------------------

This feels very nice to use!

::: devtools/client/themes/markup.css
@@ +36,5 @@
>    background: none;
>  }
>  
> +html.dragging {
> +  overflow-x: hidden;

So, I like how simple this is and how it solves your issue. But is this going to cause a problem for very deeply nested DOM trees?
What if I start dragging node that's near the root, and drag it down, towards a very deep area of the tree, so deep that nodes start to be hidden beyond the viewport, to the right.
This may easily happen if your markup-view is narrow, or if you're using the browser toolbox to inspect the browser.

Update: I just realized that we don't support horizontal auto-scrolling today anyway when drag/dropping. So this is not an issue for this bug (maybe for a follow-up or something).

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #8)
> The one thing I'm having trouble figuring out is how to use the browser test
> to control
> the height of the element. There are a lot of helper functions that I'm
> having trouble
> figuring out where they come from.
Right, this can be a little confusing at first, lot's of things get loaded without being very explicitly mentioned in the code.
So, if I take the example of this test: browser_markup_dragdrop_autoscroll.js
This test is in devtools/client/inspector/markup/test/, and the important file in this test directory is browser.ini that defines the various tests to be run.
browser.ini defines a special support-file named head.js. This will be loaded in the test's scope before it starts.
If you look at devtools/client/inspector/markup/test/head.s, you'll see it defines a lot of helper functions you can freely use in your test.
Now, it also loads another script using loadSubScript (at the top).
This script is devtools/client/inspector/test/head.js and is the general inspector-related head.js we use for all inspector tests (rule-view, computed-view, markup-view, ...).
So, any helper function in that head.js can also be used.
But if you look in this head.js, you'll see it also uses loadSubScript (near the top) to load yet another file: devtools/client/framework/test/shared-head.js
This is our general devtools shared head.js that we use in (almost) all test directories.
This file contains very common helper functions
> I'd appreciate a little direction on how
> to safely
> change the toolbox height. Right now it's a pretty small window when I test
> it on this
> machine as it is.
There's one test I know that does this already: devtools\client\framework\test\browser_toolbox_hosts_size.js
It looks like it just sets the corresponding pref before opening the toolbox (don't forget to use clearUserPref at the end of the test if you do this).

Comment 11

[Greg Tatum [:gregtatum]]

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

Comment 12

[Greg Tatum [:gregtatum]]

Attachment: Make autoscroll distance depend on the document height

I implemented the test that pbro suggested above.

It looks like the pref is not correctly being cleared between tests on Linux. I'm getting
some errors in other drag drop markup tests. Looking at the screenshot it looks like
the toolbox size is being carried over even if I clear the pref, which is incorrect.

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/61d20b01416093b480070775783d43171a13d22f809aec88866fda5c63f1dd3549e2a8b051358f020ac6f659e4938084f935c2236fc36b4eb8f952e14669c0a1

I'm still examining this to try and figure it out.

Comment 13

[Greg Tatum [:gregtatum]]

Attachment: Set and clear preferences bug.

It looks like prefs aren't working as intended.

Steps to reproduce:

* Install this patch.
* Run:
  ./mach mochitest \
    devtools/client/inspector/markup/test/browser_markup_dragdrop_autoscroll_02.js \
    devtools/client/inspector/markup/test/browser_markup_dragdrop_reorder.js \
    | grep '@@@'
* This will run the autoscroll and reorder tests in the proper order.

Actual results:

I added some custom logging to observe values of the toolbox preference height,
and the actual height. The output looks something like the following, but can
be reproduced locally with my patch.

'browser_markup_dragdrop_autoscroll_02.js'

 * getIntPref for toolbox height: 250
 * setIntPref for toolbox height: 150
 * iframe height is actually: 150
 * Run tests...
 * clearUserPref for toolbox height
 * getIntPref for toolbox height: 250

'browser_markup_dragdrop_reorder.js'

 * getIntPref for toolbox height: 150
 * iframe height is actually: 150

If I remove the setIntPref from 'browser_markup_dragdrop_autoscroll_02.js' the
following behavior is observed:

'browser_markup_dragdrop_autoscroll_02.js'

 * getIntPref for toolbox height: 250
 * iframe height is actually: 250
 * clearUserPref for toolbox height
 * getIntPref for toolbox height: 250

'browser_markup_dragdrop_reorder.js'

* getIntPref for toolbox height: 250
* iframe height is actually: 250

Expected results:

The results for 'browser_markup_dragdrop_reorder.js' should be the same on both
runs.

I looked into the source code for prefs, but I'm not to familiar with how it all
works. I will dive into it some more.

Comment 14

[Julian Descottes [:jdescottes]]

You could try to use pushPrefEnv (for which we have a helper in shared-head.js : pushPref()). The preference will automatically be cleaned up at the end of the test. In practice you have to replace `Services.prefs.setIntPref("pref.name", value);` by `yield pushPref("pref.name", value);`

setPref / clearPref are apparently subject to race conditions, it could explain your issue.

Comment 15

[Greg Tatum [:gregtatum]]

Woot, thanks Julian. That did the trick.

Comment 16

[Greg Tatum [:gregtatum]]

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

Comment 17

[Greg Tatum [:gregtatum]]

Attachment: Make autoscroll distance depend on the document height

This should address the review comments and add the additional height-based test.
I re-based and pushed to try again because the last try push had some odd failures
that didn't look related to my patch.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8766353 [details] [diff] [review]
Make autoscroll distance depend on the document height

Review of attachment 8766353 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/markup/markup.js
@@ +1894,5 @@
>      this.node = node;
>      this.undo = this.markup.undo;
>      this.win = this.markup._frame.contentWindow;
>      this.id = "treeitem-" + markupContainerID++;
> +    this.htmlElt = this.win.document.documentElement;

There's a destroy method in this class where you should also add:
this.htmlElt = null;

Comment 19

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bda9c99a4cc

Comment 20

[Greg Tatum [:gregtatum]]

Attachment: Make autoscroll distance depend on the document height

I added the `= null` to the destroy method.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0a3a72e9ecba
Make autoscroll distance depend on the document height r=pbro

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a3a72e9ecba