Closed
Bug 1127423
Opened 10 years ago
Closed 9 years ago
When selecting an element in markup view, the window gets scrolled horizontally
Categories
(DevTools :: Inspector, defect, P3)
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
Assignee | ||
Updated•10 years ago
|
Blocks: DevToolsPaperCuts
Assignee | ||
Comment 1•10 years ago
|
||
Before selecting a new element
Assignee | ||
Comment 2•10 years ago
|
||
After selecting a new element. Notice that the markup view is scrolled way to the right.
Assignee | ||
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Whiteboard: [devedition-40]
Updated•10 years ago
|
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Comment 6•10 years ago
|
||
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
Thanks for the review
Attachment #8603621 -
Attachment is obsolete: true
Attachment #8603649 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
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
Assignee | ||
Comment 11•9 years ago
|
||
There's some orange on the try push, but it all looks unrelated
Keywords: checkin-needed
Assignee | ||
Comment 12•9 years ago
|
||
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]
Assignee | ||
Comment 13•9 years ago
|
||
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]
Assignee | ||
Comment 14•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0a10db771035
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0a10db771035
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Updated•9 years ago
|
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•