Closed Bug 976559 Opened 6 years ago Closed 5 years ago

Splitter in narrow-width style editor is inconsistent with other panels

Categories

(DevTools :: Style Editor, defect)

defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: bgrins, Assigned: linclark)

References

Details

Attachments

(6 files, 2 obsolete files)

Attached image styleeditor-narrow.png
See screenshot.  The splitview-portrait-resizer is much thicker than the normal devtools splitter.
No longer blocks: 957117
Depends on: 957117
Assignee: nobody → lin.w.clark
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
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
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+
Attached patch Bug976559.patch (obsolete) — Splinter Review
Updating reviewer.
Attachment #8637955 - Attachment is obsolete: true
Attachment #8637967 - Flags: review?(bgrinstead)
Attached image 700px original
Attached image 550px original
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+
Status: NEW → ASSIGNED
Attached patch Bug976559.patchSplinter Review
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)
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9727cedcfa9b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.