Closed Bug 1389094 Opened 7 years ago Closed 7 years ago

Optimize Bookmarks and History sidebars for touch

Categories

(Firefox :: Toolbars and Customization, defect, P1)

Product:
Firefox
Component:
Toolbars and Customization
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- verified

People

(Reporter: emanx995, Assigned: nhnt11)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(1 file, 1 obsolete file)

Bug 1389094 - Increase height of sidebar search bar and tree items in touch mode.
59 bytes, text/x-review-board-request
dao
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170810100255

Steps to reproduce:

In the latest Nightlies in Tablet mode on a Windows 10 touchscreen device: 
   -Tap a folder in the Bookmarks Toolbar
   -Open the Bookmarks or History Sidebar


Actual results:

Bookmarks Toolbar folder menu list is smaller and harder to tap with your finger compared to the rest of the UI

Sidebar trees and search box are smaller and harder to tap with your finger compared to the rest of the UI


Expected results:

The Bookmark Toolbar folder menus and the Bookmark and History Sidebar trees should be bigger or have more spacing so they are easier to use on a touch screen and match the rest of the Firefox UI when Touch Density is enabled 

The main menus and context menus were optimized for touch in Bug 1371219, and the Bookmark toolbar itself is being optimized for touch in Bug 1388794, but I didn't see a bug filed for this so I thought I'd submit.
Component: Untriaged → Toolbars and Customization
Whiteboard: [photon-visual][triage]
Blocks: fx-touch
Flags: qe-verify+
Priority: -- → P4
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Comment on attachment 8910569 [details]
Bug 1389094 - Increase spacing in the sidebar in touch mode.

This is a proof of concept patch that makes the sidebar listen to the uidensity pref, and set an attribute on the documentElement which can then be used in CSS to style the sidebar for touch mode.

Marco, flagging you for feedback since you seem to have touched this file a lot before.
Attachment #8910569 - Flags: feedback?(mak77)
Attachment #8910569 - Flags: feedback?(dao+bmo)
Comment on attachment 8910569 [details]
Bug 1389094 - Increase spacing in the sidebar in touch mode.

Please see comment 2.
Attachment #8910569 - Flags: feedback?(mak77)
Attachment #8910569 - Flags: feedback?(dao+bmo)
Assignee: nobody → nhnt11
Status: UNCONFIRMED → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Ever confirmed: true
Priority: P4 → P1
Meh, thinking about it, it doesn't sound ideal to put this "automatically" running code in a utils file. I'll wait feedback though.
Comment on attachment 8910569 [details]
Bug 1389094 - Increase spacing in the sidebar in touch mode.

https://reviewboard.mozilla.org/r/182018/#review187548

The bug comment 0 also points out menus opened from folders in the bookmarks toolbar... but this patch doesn't handle that? Expected? Already fixed elsewhere?

::: browser/components/places/content/sidebarUtils.js:9
(Diff revision 2)
>  
>  Components.utils.import("resource://gre/modules/AppConstants.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +var prefBranch = Services.prefs.getBranch("browser.");

you could probably add something to the onload handler of bookmarksPanel.xul and history-panel.xul to do initialization, even in sidebarUtils... though at that point I wonder what happens to all the other Places tree views in the product:
1. the Library window views
2. the star panel folder picker

Maybe we need something that applies in general to all the places tree views? (they are managed by tree.xml and treeview.js in browser/components/places).

For prefs you should use defineLazyPreferenceGetter... Though I wonder why we don't have a broader browser observer/getter for uidensity? There should be something in browser easily accessible without everyone implementing their own accessor.
Attachment #8910569 - Flags: feedback?(mak77)
Iteration: 57.3 - Sep 19 → ---
Comment on attachment 8910569 [details]
Bug 1389094 - Increase spacing in the sidebar in touch mode.

We should abstract away the code reading browser.uidensity into a JSM so browser.xul can share it with other documents.
Attachment #8910569 - Flags: feedback?(dao+bmo)
Also, this seems to be a duplicate of bug 1391625 (which, interestingly, we decided not to do for 57), or vice versa.
Attachment #8910569 - Attachment is obsolete: true
Comment on attachment 8916430 [details]
Bug 1389094 - Increase height of sidebar search bar and tree items in touch mode.

https://reviewboard.mozilla.org/r/187556/#review192562

::: browser/base/content/browser.js:5732
(Diff revision 1)
>        mode = this.getCurrentDensity().mode;
>      }
>  
> -    let doc = document.documentElement;
> +    let docs = [
> +      document.documentElement,
> +      SidebarUI.browser.contentDocument.documentElement,

Turns out we have easy access to the sidebar document so I decided to not bother with refactoring gUIDensity and just make it operate on both documents.

::: browser/themes/windows/places/places.css:26
(Diff revision 1)
>    margin: 0;
>  }
>  
> +:root[uidensity=touch] #search-box,
> +:root[uidensity=touch] .sidebar-placesTreechildren::-moz-tree-row {
> +  height: 32px;

32px felt good to me, but maybe this should be in rems?
Hmm, I haven't opened my laptop since I submitted this new patch but I think we need to call gUIDensity.update() from sidebarUtils.js to make sure the attribute gets applied when switching between different sidebar modes.
Comment on attachment 8916430 [details]
Bug 1389094 - Increase height of sidebar search bar and tree items in touch mode.

https://reviewboard.mozilla.org/r/187556/#review193162

::: browser/base/content/browser.js:5750
(Diff revision 2)
> -    }
> +      }
> +    }
> +    let tree = SidebarUI.browser.contentDocument.querySelector(".sidebar-placesTree");
> +    if (tree) {
> +      // For some reason, the tree items don't update their styles without changing
> +      // some property on the parent tree element, like background-color or border.

As discussed, please file a bug on this and mention it in this comment.

::: browser/base/content/browser.js:5752
(Diff revision 2)
> +    let tree = SidebarUI.browser.contentDocument.querySelector(".sidebar-placesTree");
> +    if (tree) {
> +      // For some reason, the tree items don't update their styles without changing
> +      // some property on the parent tree element, like background-color or border.
> +      tree.style.border = "1px";
> +      tree.style.border = "0";

"" rather than "0"

::: browser/themes/windows/places/places.css:26
(Diff revision 2)
>    margin: 0;
>  }
>  
> +:root[uidensity=touch] #search-box,
> +:root[uidensity=touch] .sidebar-placesTreechildren::-moz-tree-row {
> +  height: 32px;

Can you use min-height?
Attachment #8916430 - Flags: review?(dao+bmo)
Depends on: 1407399
Comment on attachment 8916430 [details]
Bug 1389094 - Increase height of sidebar search bar and tree items in touch mode.

https://reviewboard.mozilla.org/r/187556/#review193948

::: commit-message-5eba1:1
(Diff revision 3)
> +Bug 1389094 - Increase height of sidebar search bar and tree items in touch mode on Windows. r=dao

Why is this specific to Windows? Touch mode generally works across platforms.
Attachment #8916430 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #16)
> Comment on attachment 8916430 [details]
> Bug 1389094 - Increase height of sidebar search bar and tree items in touch
> mode on Windows.
> 
> https://reviewboard.mozilla.org/r/187556/#review193948
> 
> ::: commit-message-5eba1:1
> (Diff revision 3)
> > +Bug 1389094 - Increase height of sidebar search bar and tree items in touch mode on Windows. r=dao
> 
> Why is this specific to Windows? Touch mode generally works across platforms.

I've added rules for Linux and MacOS in the new patch.

On Mac, I've only increased the height of the tree rows and not the search box. Simply changing the min-height or padding of the search bar on was ugly and resulted in a blurry focus ring and stretched search icon. Seems to be due to the way -moz-appearance: searchfield works. We could manually style the search bar but I think that broadens the scope of this bug too much. Also, we don't expose the Touch density setting in Customize mode on Mac, so meh.

I previously thought that we don't expose the Touch density setting on Linux either - that's why I only added rules for Windows - but I was wrong about that.
(In reply to Nihanth Subramanya [:nhnt11] from comment #18)
> Also, we don't expose the Touch density setting in Customize mode on Mac, so
> meh.

I hadn't realized that. In this case, I guess there's no point in changing browser/themes/osx/places/places.css.
But you can run macOS on touch devices (that's why it does not make sense that this setting is not exposed on macOS) and change the setting via about:config, this works also on macOS. So please don't ignore macOS…
Comment on attachment 8916430 [details]
Bug 1389094 - Increase height of sidebar search bar and tree items in touch mode.

https://reviewboard.mozilla.org/r/187556/#review194404
Attachment #8916430 - Flags: review?(dao+bmo) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c82578ae310
Increase height of sidebar search bar and tree items in touch mode. r=dao
https://hg.mozilla.org/mozilla-central/rev/4c82578ae310
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Summary: Optimize Bookmarks Toolbar menus and Sidebar for touch → Optimize Bookmarks and History sidebars for touch
I verified this issue using Nightly 58.0a1 with build ID 20170810100255 on Windows 10 x64 Surface Pro 4
I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Flags: qe-verify+
Just a headsup, I've filed a separate bug for the Bookmarks folder menus I mentioned in comment 0, as well as a few other menus I noticed still need to be optimized for touch - Bug 1409698
I would be surprised if accessing SidebarUI.browser.contentDocument.documentElement during startup didn't force the creation of the sidebar browser and an about:blank load in it, even when the sidebar is hidden. Likely regressing bug 1369751.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: