Closed Bug 1127423 Opened 5 years ago Closed 5 years ago

When selecting an element in markup view, the window gets scrolled horizontally

Categories

(DevTools :: Inspector, defect, P3)

x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(3 files, 4 obsolete files)

This is annoying, especially in the Browser Toolbox.  Get into a reasonably nested element and select a child.  The whole viewport shifts to the right.

I think it's because scrollIntoViewIfNeeded [0] scrolls horizontally.  In this case, I don't think that's what we want.

[0]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/LayoutHelpers.jsm#224
Attached image before.png
Before selecting a new element
Attached image after.png
After selecting a new element.  Notice that the markup view is scrolled way to the right.
Patrick, looks like the markup view is the only place using this function: https://dxr.mozilla.org/mozilla-central/search?q=scrollIntoViewIfNeeded&redirect=true.  I'm inclined to just strip out any horizontal scrolling from this function, as I'm not sure if we ever want this to happen.  What do you think?
Flags: needinfo?(pbrosset)
This is *very* annoying indeed, it makes it really hard to not loose track of the nodes you're looking at in the browser toolbox.

I agree to just remove the auto horizontal scrolling part.
Flags: needinfo?(pbrosset)
I was hoping this was going to be a really quick fix, but there are two weird things going on, so I'm just going to stash this WIP here:

1) the horizontal scrolling issue still seems to happen when clicking on the element -- even if I completely comment out scrollIntoViewIfNeeded.  Not that we shouldn't proceed with changing this behavior (it could still help when in element picking mode), but I can't find any code in the markup view that is causing this scrolling to happen.
2) Some of the assertions in the existing test simply never run right now, and when you make them run, they fail: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/test/browser_layoutHelpers.js#89.  It's waiting for a load event on an iframe, but never yielding for it, and also doesn't handle the case where the frame has already loaded when this happens.
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Summary: When selecting an element in markup view, the window gets scrolled to the horizontally → When selecting an element in markup view, the window gets scrolled horizontally
Attached patch scrollintoview.patch (obsolete) — Splinter Review
Hi Ryan, I would request review from Patrick but his queue is very big right now.  This is just a change to a shared/ utility for scrolling into view.  I believe this used to be used for the highlighter before it was remoted so it handled lots of extra stuff (horizontal scrolling, iframes).  But now it's only used client side in the toolbox and we don't need this functionality.

I guess you could make a case that some day we will want this back, but in the meantime I think it's best to just remove the code.  I've done this and updated the test (which wasn't actually testing a lot before hand).  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b5efd0498f.
Assignee: nobody → bgrinstead
Attachment #8558656 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8603621 - Flags: review?(jryans)
Comment on attachment 8603621 [details] [diff] [review]
scrollintoview.patch

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

::: toolkit/devtools/LayoutHelpers.jsm
@@ +214,5 @@
>      }
>      return node;
>    },
>  
> +  /**`

Nit: remove `
Attachment #8603621 - Flags: review?(jryans) → review+
Attached patch scrollintoview-r=jryans.patch (obsolete) — Splinter Review
Thanks for the review
Attachment #8603621 - Attachment is obsolete: true
Attachment #8603649 - Flags: review+
Attached patch scrollintoview-r=jryans.patch (obsolete) — Splinter Review
Had to do some rounding on the test for off by .5px errors on various platforms.  New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=673e051bbfef
Attachment #8603649 - Attachment is obsolete: true
There's some orange on the try push, but it all looks unrelated
Keywords: checkin-needed
remote:   https://hg.mozilla.org/integration/fx-team/rev/f407ab8f7129
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [fixed-in-fx-team][devedition-40][difficulty=easy]
Backed this out over suspicion of causing test failures on browser/devtools/shared/test/browser_mdn-docs-01.js like https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=f407ab8f7129

https://hg.mozilla.org/integration/fx-team/rev/aa4eac3a4336
Whiteboard: [fixed-in-fx-team][devedition-40][difficulty=easy] → [devedition-40][difficulty=easy]
Just had to load the test content in a host instead of a tab.  As a bonus this also should also fix any CPOW warnings the test was throwing.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93e2c70ffe85
Attachment #8603684 - Attachment is obsolete: true
Attachment #8603734 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0a10db771035
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.