Closed Bug 1127423 Opened 5 years ago Closed 5 years ago

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


(DevTools :: Inspector, defect, P3)



(firefox40 fixed)

Firefox 40
Tracking Status
firefox40 --- fixed


(Reporter: bgrins, Assigned: bgrins)


(Blocks 1 open bug)


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


(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.

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:  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:  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:
Assignee: nobody → bgrinstead
Attachment #8558656 - Attachment is obsolete: true
Attachment #8603621 - Flags: review?(jryans)
Comment on attachment 8603621 [details] [diff] [review]

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:
Attachment #8603649 - Attachment is obsolete: true
There's some orange on the try push, but it all looks unrelated
Keywords: checkin-needed
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
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:
Attachment #8603684 - Attachment is obsolete: true
Attachment #8603734 - Flags: review+
Keywords: checkin-needed
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.