Closed Bug 1469287 Opened 5 years ago Closed 5 years ago

Implement new shared tree styling

Categories

(Toolkit :: Themes, defect, P3)

Product:
Toolkit
Component:
Themes
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

Details

(Whiteboard: [ntim-intern-project])

Attachments

(1 file)

Bug 1469287 - Implement new shared tree styling.
59 bytes, text/x-review-board-request
dao
: review+
Details 
As discussed as the SF all hands, the same photon styling should be used across platforms. See https://firefoxux.github.io/people/shorlander/ for mockups.
Assignee: nobody → ntim.bugs
Blocks: 1418602
Priority: -- → P3
Depends on: 1466826
Whiteboard: [ntim-intern-project]
Blocks: 1400266
Depends on: 1470920
No longer depends on: 1470920
Depends on: 1471542
Tested this on all 3 platforms, no issues found so far.
Comment on attachment 8988190 [details]
Bug 1469287 - Implement new shared tree styling.

https://reviewboard.mozilla.org/r/253448/#review260320

Focus behavior doesn't seem where we need it to be. Selected & focused needs to be styled differently than selected & unfocused. Tested in the Library on Ubuntu.

We used to have a focus ring for this on Linux, and I believe a different background color to distinguish these states on Windows -- I think the latter is preferable.
Attachment #8988190 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #6)
> Comment on attachment 8988190 [details]
> Bug 1469287 - Implement new shared tree styling.
> 
> https://reviewboard.mozilla.org/r/253448/#review260320
> 
> Focus behavior doesn't seem where we need it to be. Selected & focused needs
> to be styled differently than selected & unfocused. Tested in the Library on
> Ubuntu.
> 
> We used to have a focus ring for this on Linux, and I believe a different
> background color to distinguish these states on Windows -- I think the
> latter is preferable.

Re-added the old focusring.
No longer blocks: 1400266
Depends on: 1400266
(In reply to Tim Nguyen :ntim from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > Comment on attachment 8988190 [details]
> > Bug 1469287 - Implement new shared tree styling.
> > 
> > https://reviewboard.mozilla.org/r/253448/#review260320
> > 
> > Focus behavior doesn't seem where we need it to be. Selected & focused needs
> > to be styled differently than selected & unfocused. Tested in the Library on
> > Ubuntu.
> > 
> > We used to have a focus ring for this on Linux, and I believe a different
> > background color to distinguish these states on Windows -- I think the
> > latter is preferable.
> 
> Re-added the old focusring.

Like I said I think different background colors are preferable, and introducing the focus ring on Windows and Mac would arguably be a regression.
Attachment #8988190 - Flags: review?(dao+bmo)
Depends on: 1472276
Däo, can we move forward with this and work around bug 1472276 with some Linux specific CSS ?
Flags: needinfo?(dao+bmo)
(In reply to Tim Nguyen :ntim from comment #17)
> Däo, can we move forward with this and work around bug 1472276 with some
> Linux specific CSS ?

It would be better to fix this properly in bug 1472276 with a fallback to highlight for problematic color combinations.
Flags: needinfo?(dao+bmo)
Depends on: 1481270
No longer depends on: 1481270
I've rebased the patch, so it should be ready for review now that bug 1472276 has landed.
Comment on attachment 8988190 [details]
Bug 1469287 - Implement new shared tree styling.

https://reviewboard.mozilla.org/r/253448/#review269414

Trees look more or less unstyled with this on Linux. I haven't looked into why, just applied the patch to try it. Might just be a typo somewhere.
Attachment #8988190 - Flags: review?(dao+bmo) → review-
Comment on attachment 8988190 [details]
Bug 1469287 - Implement new shared tree styling.

https://reviewboard.mozilla.org/r/253448/#review269430

::: browser/themes/osx/places/places.css:56
(Diff revision 15)
>    margin-inline-end: 6px;
>  }
>  
>  .sidebar-placesTreechildren::-moz-tree-twisty(selected),
>  .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
>    color: #fff;

Is this still needed?

::: browser/themes/osx/places/places.css:73
(Diff revision 15)
> -}
> -
>  @media (-moz-mac-yosemite-theme) {
>    .sidebar-placesTreechildren::-moz-tree-twisty(selected),
>    .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
>      color: -moz-dialogtext;

And this?
Attachment #8988190 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #25)
> Comment on attachment 8988190 [details]
> Bug 1469287 - Implement new shared tree styling.
> 
> https://reviewboard.mozilla.org/r/253448/#review269430
> 
> ::: browser/themes/osx/places/places.css:56
> (Diff revision 15)
> >    margin-inline-end: 6px;
> >  }
> >  
> >  .sidebar-placesTreechildren::-moz-tree-twisty(selected),
> >  .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
> >    color: #fff;
> 
> Is this still needed?
> 
> ::: browser/themes/osx/places/places.css:73
> (Diff revision 15)
> > -}
> > -
> >  @media (-moz-mac-yosemite-theme) {
> >    .sidebar-placesTreechildren::-moz-tree-twisty(selected),
> >    .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
> >      color: -moz-dialogtext;
> 
> And this?

I'm not sure. This code seems to be taking in account that -moz-mac-source-list-selection/-moz-mac-active-source-list-selection are different on MacOS 10.9 vs. 10.10+. I'd have to test on MacOS 10.9 to see if I can remove it, unfortunately I don't have access to this OS to check.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c8c488989a62
Implement new shared tree styling. r=dao
(In reply to Tim Nguyen :ntim from comment #26)
> (In reply to Dão Gottwald [::dao] from comment #25)
> > Comment on attachment 8988190 [details]
> > Bug 1469287 - Implement new shared tree styling.
> > 
> > https://reviewboard.mozilla.org/r/253448/#review269430
> > 
> > ::: browser/themes/osx/places/places.css:56
> > (Diff revision 15)
> > >    margin-inline-end: 6px;
> > >  }
> > >  
> > >  .sidebar-placesTreechildren::-moz-tree-twisty(selected),
> > >  .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
> > >    color: #fff;
> > 
> > Is this still needed?
> > 
> > ::: browser/themes/osx/places/places.css:73
> > (Diff revision 15)
> > > -}
> > > -
> > >  @media (-moz-mac-yosemite-theme) {
> > >    .sidebar-placesTreechildren::-moz-tree-twisty(selected),
> > >    .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
> > >      color: -moz-dialogtext;
> > 
> > And this?
> 
> I'm not sure. This code seems to be taking in account that
> -moz-mac-source-list-selection/-moz-mac-active-source-list-selection are
> different on MacOS 10.9 vs. 10.10+. I'd have to test on MacOS 10.9 to see if
> I can remove it, unfortunately I don't have access to this OS to check.

Yeah, -moz-mac-source-list-selection/-moz-mac-active-source-list-selection are different on MacOS 10.9 vs. 10.10+. And the selection foreground color is always white on 10.9.
https://hg.mozilla.org/mozilla-central/rev/c8c488989a62
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1484946
Blocks: 1484946
No longer depends on: 1484946
Depends on: 1485830
Depends on: 1487397
Yet another UX change to undo. Yay.

For those like me, who can't bear such ridiculously large rows, I had to add the following to my userChrome.css, to get back my screen estate for the Bookmarks and History sidebars:

    .sidebar-placesTreechildren::-moz-tree-row {
        min-height: 22px !important; /* default: 24px */
    }
To also fix e.g. the Library, I have switched to the following rule, with a broader selector, and also simpler:

    treechildren::-moz-tree-row {
        min-height: 22px !important; /* default: 24px */
    }
Depends on: 1510539
Regressions: 1581578
You need to log in before you can comment on or make changes to this bug.