Optimize Bookmarks and History sidebars for touch

VERIFIED FIXED in Firefox 58

Status

()

P1
normal
VERIFIED FIXED
a year ago
9 months ago

People

(Reporter: emanx995, Assigned: nhnt11)

Tracking

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

57 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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

Updated

a year ago
Whiteboard: [photon-visual][triage]

Updated

a year ago
Blocks: 1334161
Flags: qe-verify+
Priority: -- → P4
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
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
(Assignee)

Comment 5

a year ago
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 6

a year ago
mozreview-review
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.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1391625
Blocks: 1352356
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8910569 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
mozreview-review
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?
(Assignee)

Comment 12

a year ago
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 hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
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)
(Assignee)

Updated

a year ago
Depends on: 1407399
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
(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.
Comment hidden (mozreview-request)
(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.

Comment 21

a year ago
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 22

a year ago
mozreview-review
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+

Comment 23

a year ago
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
Last Resolved: a year 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+
(Reporter)

Comment 26

a year ago
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.