Closed Bug 752850 Opened 8 years ago Closed 7 years ago

[Responsive Mode] It should be possible to hide the scrollbars

Categories

(DevTools :: General, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: paul, Assigned: paul)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 11 obsolete files)

To reproduce a "mobile experience", we should let the user hide the scrollbars.
Blocks: 778169
I think it is now possible to just display:none the scrollbars. See bug 737003.
Apparently, it doesn't work. I started a thread about that on dev-platform:
https://groups.google.com/d/topic/mozilla.dev.platform/wGMjWhHxKtE/discussion
We found a solution:

let win = gBrowser.contentWindow;
let gIOService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
let windowUtils = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils);
let uri = gIOService.newURI('data:text/css,@namespace url("http://www.w3.org/1999/xhtml");@namespace xul url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");html xul|scrollbar {display:none}', null, null);
windowUtils.loadSheet(uri, windowUtils.AGENT_SHEET);
gBrowser.selectedBrowser.setAttribute("style", "display:none");
window.getComputedStyle(gBrowser.selectedBrowser).display;
gBrowser.selectedBrowser.removeAttribute("style");
So now we have a working mechanism, we need to figure out what we could do for the user interface.

We could have a checkbox. But I don't like checkboxes.

We could hide the scrollbars automatically when we are below 600px. Does sound terrible.

Any idea?
Nice work.

Why 600px? Automatically => :-(

What about some kind of switch? With :

Scrollbars : YES | NO

It's not really part of the default UX of Firefox though, isn't it?

BTW the same could be nice for orientation. With landscape and portrait.
Attached file Scratchpad (obsolete) —
This scratchpad adds small scrollbars that overlaps content (like on mobile browser).

Styling scrollbars with CSS appears to be a little fragile. I didn't manage to get horizontal scrollbars. I also tried to get the rounded thumbs, but didn't succeed. But the current approach is satisfying.
(In reply to hsablonniere from comment #5)
> Why 600px?

I don't know. Maybe we could just hide the scrollbar (or get the overlapping scrollbar) as soon as we enter the responsive mode.

> Automatically => :-(

Why not?

> What about some kind of switch? With :
> 
> Scrollbars : YES | NO
> 
> It's not really part of the default UX of Firefox though, isn't it?

Nope. But we can build one.

> BTW the same could be nice for orientation. With landscape and portrait.

Maybe.

So yeah… as you know, I'm not keen on adding UI elements.

I see 2 options:

1) we know that the scrollbar is not really useful in this specific case (testing responsive design), so let's just hide it as soon as we enter this mode.
2) we think it's better to have the choice, so then let's put a checkbox.
(the current scratchpad is not working very well when the scrollbar is inside the content)
Attached image ugly scrolbars (obsolete) —
looks ugly on windows :(
Attached patch v0.1 (obsolete) — Splinter Review
Todo:
- windows & linux support
- support reload of the top level window and inner windows
- support dynamic creation of windows
Comment on attachment 665996 [details] [diff] [review]
v0.1

Hubert, can you try this patch and tell me what you think?
Attachment #665996 - Flags: feedback?(hubert.sablonniere)
This works on windows : 
http://pastebin.mozilla.org/1848168

Though I have not tested it on a rtl locale to properly see what -moz-box-align: start; could have impacted.
Attached patch v1 (obsolete) — Splinter Review
Attachment #665996 - Attachment is obsolete: true
Attachment #665996 - Flags: feedback?(hubert.sablonniere)
Optimizer, I didn't include your changes. Can I ask you to modify this patch to make it work on Windows? If you can just add a patch I can `hg qfold` into this one, that would help a lot :)

Let me know!
Comment on attachment 666904 [details] [diff] [review]
v1

Joe, this has been tested on mac only for now (we know it doesn't work well on windows). Some more patches are coming.
Attachment #666904 - Flags: review?(jwalker)
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #666904 - Attachment is obsolete: true
Attachment #666904 - Flags: review?(jwalker)
Attachment #666912 - Flags: review?(jwalker)
Comment on attachment 666912 [details] [diff] [review]
v1.1

>--- a/browser/themes/winstripe/jar.mn
>+++ b/browser/themes/winstripe/jar.mn

>         skin/classic/aero/browser/devtools/tools-icons-small.png     (devtools/tools-icons-small.png)
>+        skin/classic/browser/devtools/floating-scrollbars.css        (devtools/floating-scrollbar.scss)

This is broken.
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #666912 - Attachment is obsolete: true
Attachment #666912 - Flags: review?(jwalker)
Attachment #666917 - Flags: review?(jwalker)
Comment on attachment 666912 [details] [diff] [review]
v1.1

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

Did you miss out browser/themes/gnomestripe/jar.mn

::: browser/devtools/shared/FloatingScrollbars.jsm
@@ +5,5 @@
> +"use strict";
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +
> +const EXPORTED_SYMBOLS = [ "SwitchToFloatingScrollbars", "SwitchToNativeScrollbars" ];

I think I would have done

    Scrollbars = { useFloating: ..., useNative: ... };

I'm not sure that it's important, and I wouldn't r- because of it, but it does feel more contained to me.

I do think that if we're exporting functions then we should use lower initials so they don't get confused with constructors. So at the very least I think we should do

    const EXPORTED_SYMBOLS = [ "switchToFloatingScrollbars", "switchToNativeScrollbars" ];

Although 

    const EXPORTED_SYMBOLS = [ "Scrollbars" ];

feels better still.

@@ +40,5 @@
> +    mgr.reset();
> +  }
> +}
> +
> +function _ScrollbarManager(aTab) {

Since this isn't exported, do we need the initial _?

::: browser/themes/gnomestripe/devtools/floating-scrollbars.css
@@ +1,4 @@
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +
> +scrollbar {
> +  -moz-appearance: none !important;

I think we should document why we need to use !important. Or at the very least why some of these properties need it but not others.
Attachment #666912 - Attachment is obsolete: false
I prefer to avoid something like "scrollbar". I want the name of the exported symbol(s) to be very explicit as it only does one thing.

Ok for the lower case and the "_".

We don't document the "!important" anywhere. See for example https://mxr.mozilla.org/mozilla-central/source/mobile/xul/themes/core/content.css or http:mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/content.css
Attached patch v1.3 (obsolete) — Splinter Review
Attachment #666912 - Attachment is obsolete: true
Attachment #666917 - Attachment is obsolete: true
Attachment #666917 - Flags: review?(jwalker)
Attachment #666922 - Flags: review?(jwalker)
Attached patch v1.4 (obsolete) — Splinter Review
Attachment #666922 - Attachment is obsolete: true
Attachment #666922 - Flags: review?(jwalker)
(typo in jar.man)
(In reply to Paul Rouget [:paul] from comment #23)
> We don't document the "!important" anywhere. See for example
> https://mxr.mozilla.org/mozilla-central/source/mobile/xul/themes/core/
> content.css or
> http:mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/content.css

I'm still left wondering why we needed !important, and why we needed it some places and not others.
We could have that discussion here, but I'm guessing that I might not be the only person to benefit from the thinking.
(In reply to Joe Walker [:jwalker] from comment #27)
> I'm still left wondering why we needed !important, and why we needed it some
> places and not others.
> We could have that discussion here, but I'm guessing that I might not be the
> only person to benefit from the thinking.

I just copied the code we use for mobile. I will remove all the !important and see which ones are really useful.
Attached patch v1.5 (obsolete) — Splinter Review
removed all the important. Only useful for thumb. But I don't know why.
Attachment #666923 - Attachment is obsolete: true
Attached patch Patch v1.5 with Windows fixes (obsolete) — Splinter Review
Same as patch v1.5 for other OS than Windows. Made all the changes required for windows. Removed !importants where not required.
Attachment #665471 - Attachment is obsolete: true
Attachment #666926 - Attachment is obsolete: true
Attachment #666932 - Flags: review?(jwalker)
(I'm ok with Optmizer's changes)
Comment on attachment 666932 [details] [diff] [review]
Patch v1.5 with Windows fixes

Cancelling the review until I get my new laptop to test on Linux and Windows (apparently, this patch doesn't work at all on Linux).
Attachment #666932 - Flags: review?(jwalker)
Attached patch v1.6 (obsolete) — Splinter Review
Attachment #666932 - Attachment is obsolete: true
Comment on attachment 666945 [details] [diff] [review]
v1.7 - Fixed Linux issues

Thank you guys!
Attachment #666945 - Flags: review?(jwalker)
Attachment #666945 - Flags: review?(jwalker) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/1332149e38af
Assignee: nobody → paul
Status: NEW → ASSIGNED
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1332149e38af
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Depends on: 797335
Depends on: 798772
Should this be reopened? Currently scrollbars are shown and there's no way to turn them off.
(In reply to Chris Lord [:cwiiis] from comment #38)
> Should this be reopened? Currently scrollbars are shown and there's no way
> to turn them off.

See bug 799471.
Excuse me, Is there possible way to enable this implementation into Firefox default view? In fact, this is exactly the scrollbar style that I want on my websites, and I'm doing something similar to this for Chrome using ::-webkit-scrollbar rules, because this looks way better than ugly windows classic gray scrollbars.
This could be possible a fix for bug 77790, bug 547260, bug 384458, bug 21890, bug 259640, and even bug 590945 can be related somehow.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.