Closed
Bug 976559
Opened 10 years ago
Closed 9 years ago
Splitter in narrow-width style editor is inconsistent with other panels
Categories
(DevTools :: Style Editor, defect)
DevTools
Style Editor
Tracking
(firefox42 fixed)
RESOLVED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: bgrins, Assigned: linclark)
References
Details
Attachments
(6 files, 2 obsolete files)
See screenshot. The splitview-portrait-resizer is much thicker than the normal devtools splitter.
Reporter | ||
Updated•10 years ago
|
Updated•9 years ago
|
Assignee: nobody → lin.w.clark
Reporter | ||
Comment 1•9 years ago
|
||
Looks like this is the .splitview-portrait-resizer [0]. My thinking is that this should act like this inspector does [1] and use a <xul:splitter class="devtools-side-splitter"/> inside of a .devtools-responsive-container container element. If that's possible, then the fix would go into styleeditor.xul [2] and the any CSS found in [0] could be deleted. [0]: https://dxr.mozilla.org/mozilla-central/search?q=splitview-portrait-resizer&redirect=true [1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul#146 [2]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/styleeditor.xul#151
Reporter | ||
Comment 2•9 years ago
|
||
Unfortunately it might be likely that the approach in Comment 1 won't work without modification, since the split view and the responsive-design-container might have different breakpoints for the screen size when it switches layouts. If that's the case, then we could instead just update the CSS for the splitview-portrait-resizer to look like the devtools-side-splitter does (which is unfortunately split up across a few files - see common.css, toolbars.inc.css and widgets.inc.css for the relevant styles) [0]. [0]: https://dxr.mozilla.org/mozilla-central/search?q=path%3A*.css+.devtools-side-splitter&redirect=true
Assignee | ||
Comment 3•9 years ago
|
||
The attached patch is just a first pass. I don't really have a full understanding of the whole system, so please feel free to give me feedback if this is the wrong direction. I liked the approach discussed in Comment 1. When I started trying to make it work, I found that there was already a splitter that was being hidden at the 550px mark. My solution makes a small change in functionality. With the original break point, the view switches to 3 columns at 550px. In the new version, the view doesn't switch to 3 columns until 750px. I don't know whether this is a problem.
Attachment #8637955 -
Flags: review+
Assignee | ||
Comment 4•9 years ago
|
||
Updating reviewer.
Attachment #8637955 -
Attachment is obsolete: true
Attachment #8637967 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Reporter | ||
Comment 9•9 years ago
|
||
Comment on attachment 8637967 [details] [diff] [review] Bug976559.patch Review of attachment 8637967 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch (and the gifs)! It really helps clear up what needs to be done here, and I found just a few minor updates: * We can remove all references in CSS to splitview-portrait-resizer since the element is removed: https://dxr.mozilla.org/mozilla-central/search?q=splitview-portrait-resizer&redirect=true * Can remove the splitview-landscape-splitter class on the remaining splitter in styleeditor.xul * Can remove what appears to be an unused class in splitview.css: splitview-landscape-resizer There's an issue with the patch applied when loading a page with a bunch of style sheets (like cnn.com) in a window between 550px and 700px. The style sheet list fills up the whole screen and you can't scroll down to the splitter. I believe we need to change LANDSCAPE_MEDIA_QUERY to "(min-width: 701px)" and also update the corresponding min-width: 551px to min-width: 701px inside splitview.css. This way we will be consistently treating the sizes as something like portait <= 700px and landscape > 700px in both the devtools-responsive styles and the splitview styles. Once you've made those changes, it'd be worth running through the tests for the style editor locally just to make sure there weren't any hardcoded accesses to the old classes / widths. You can run them with `./mach mochitest browser/devtools/styleeditor/`. ::: browser/devtools/shared/splitview.css @@ +83,5 @@ > display: none; > } > > /* portrait mode */ > +@media (max-width: 750px) { I think this should be max-width: 700px to match what the responsive container is doing
Attachment #8637967 -
Flags: review?(bgrinstead) → feedback+
Reporter | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•9 years ago
|
||
Thanks for the review and pointers, they were very helpful. I believe I made all the necessary changes. From comment 9, those changes were: * Removed all references to .splitview-portrait-resizer * Removed .splitview-landscape-splitter * Removed .splitview-landscape-resizer * Changed LANDSCAPE_MEDIA_QUERY to "(min-width: 701px)" and updated corresponding line splitview.css * Reduced the breakpoint change I had made from 750 to 700px I manually tested the issue that bgrins raised with cnn.com, and the suggested changes fixed that issue. I also ran the automated test suite and everything passed.
Attachment #8637967 -
Attachment is obsolete: true
Attachment #8638930 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 11•9 years ago
|
||
Comment on attachment 8638930 [details] [diff] [review] Bug976559.patch Review of attachment 8638930 [details] [diff] [review]: ----------------------------------------------------------------- I've applied it and tested and it seems consistent with other tools, plus it's a net removal of code so looks good to me! Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5181d27a13be
Attachment #8638930 -
Flags: review?(bgrinstead) → review+
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9727cedcfa9b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9727cedcfa9b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•