Closed Bug 1287422 Opened 6 years ago Closed 6 years ago

Don't overlap font-showall button and scrollbar in the Font Inspector

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox50 affected, firefox51 verified)

VERIFIED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox50 --- affected
firefox51 --- verified

People

(Reporter: magicp.jp, Assigned: steveck)

References

Details

(Whiteboard: [reserve-html][good taipei bug])

Attachments

(7 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160717030211

Steps to reproduce:

1. Start Nightly
2. Go to Getting Started "https://support.mozilla.org/en-US/kb/get-started-firefox-overview-main-features"
3. Open DevTools > Inspector > Fonts
4. Click font-showall button


Actual results:

font-showall button and scrollbar are overlapped.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a8ab215f15d3a875f964663a6011b5a6cba67919&tochange=453c308dcab1e3236fde0c4ee2e6d0d3d2d60dae


Expected results:

Don't overlap font-showall button and scrollbar.
Blocks: 1259819
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Component: Developer Tools: Inspector → Developer Tools: Font Inspector
Whiteboard: [devtools-html] [triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [reserve-html]
Whiteboard: [reserve-html] → [reserve-html][good taipei bug]
Priority: P3 → P2
Assignee: nobody → schung
Status: NEW → ASSIGNED
Hi Tim/magicp,

It's a simple one-liner patch for showall button's position. However, I also discovered some other differentiation between current master and version before sidebar refactoring. I'll attach the comparison image later.
Flags: needinfo?(magicp.jp)
Attachment #8774239 - Flags: feedback?(ntim.bugs)
Attached image showall.jpeg
In the previous version, the panel added a padding bottom and it could make sure there's no overlay between showall button and panel(but looks ugly). Now the panel container missed the proper boz-sizing and cause the overlay with background panel content(it also looks ugly because the button will apply opacity when hover...). Should we resume the previous style, or just remove the button hover transparent styling?
Hi Steve,
Can we move the show all button into toolbar like attached video?
Flags: needinfo?(magicp.jp)
I like magicp's suggestion. Helen, are you ok with it ?
Flags: needinfo?(hholmes)
Comment on attachment 8774239 [details] [diff] [review]
bug-1287422.patch

The fix looks good, let's see what Helen has to say about magicp's suggestion.
Attachment #8774239 - Flags: feedback?(ntim.bugs) → feedback+
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #4)
> I like magicp's suggestion. Helen, are you ok with it ?

I'm fine with this, especially if it fixes a lot of positioning issues.
Flags: needinfo?(hholmes)
(In reply to magicp from comment #3)
> Created attachment 8774253 [details]
> move-show-all-position.mp4
> 
> Hi Steve,
> Can we move the show all button into toolbar like attached video?

As you wish :)

Hi Tim, I think it might need copywriter's(or UX?) review about the string changes. Just let me know if necessary. I'll also attach the screenshot for more feedback from Helen, thanks!
Attached image showall.jpeg (obsolete) —
Hi Helen, it's the screenshot about the patch. Feel free to give any suggestion for polishing.
Attachment #8775094 - Flags: feedback?(hholmes)
Attachment #8775088 - Flags: review?(ntim.bugs)
Comment on attachment 8775088 [details]
Bug 1287422 - Don't overlap font-showall button and scrollbar in the Font Inspector.

https://reviewboard.mozilla.org/r/67404/#review64442

I'll let Helen review the string.

::: devtools/client/locales/en-US/font-inspector.dtd:8
(Diff revision 1)
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- LOCALIZATION NOTE : FILE This file contains the Font Inspector strings.
>    - The Font Inspector is the panel accessible in the Inspector sidebar. -->
>  
> -<!ENTITY showAllFonts "See all the fonts used in the page">
> +<!ENTITY showAllFonts "See all the fonts">

You'll need to update the string ID (showAllFonts -> showAllFonts1) so localizers will catch the change.

::: devtools/client/themes/fonts.css:29
(Diff revision 1)
>    padding: 0;
>    margin: 0;
>  }
>  
>  #font-showall {
>    border-radius: 0;

Can you remove the CSS rule here and use the .devtools-button class on the button ?
You'll need to add the selectors .theme-light/.theme-dark .devtools-button:not(.empty) here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#262
Comment on attachment 8775094 [details]
showall.jpeg

Thanks for attaching this!

Pleased generally with it. A few notes:

- Can we make this a blue link instead of a button? We're trying to move away from the default gray buttons in devtools visually, as a rule.
- "Show all fonts used" << can we swap it out to this string instead? I think it might be more descriptive.

Thanks!
Attachment #8775094 - Flags: feedback?(hholmes) → feedback+
Attached image showall in 3 themes (obsolete) —
Hi Helen, it's the result after applying the Tim's suggestion. Seems like we might need to remove the border or set specific border style for dark theme. And I'm not sure about the blue link you mentioned... Is that as same as the image  I attached(at the right hand side) for the inline style link?
Attachment #8775420 - Flags: feedback?(hholmes)
BTW I'm fine with opening another follow up bug for replacing the show all button with link
(In reply to Steve Chung [:steveck] from comment #12)
> Created attachment 8775420 [details]
> showall in 3 themes
> 
> Hi Helen, it's the result after applying the Tim's suggestion. Seems like we
> might need to remove the border or set specific border style for dark theme.
> And I'm not sure about the blue link you mentioned... Is that as same as the
> image  I attached(at the right hand side) for the inline style link?

Yep, that's the style of link I was referring to. Making this a link instead of a button should solve the dark/light theme problems.
Comment on attachment 8775420 [details]
showall in 3 themes

Left feedback in comment #14. I think using that style of link would be best here over the button.
Attachment #8775420 - Flags: feedback?(hholmes) → feedback+
Comment on attachment 8775088 [details]
Bug 1287422 - Don't overlap font-showall button and scrollbar in the Font Inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67404/diff/1-2/
Attachment #8775088 - Flags: review?(ntim.bugs)
Attached image showall.jpeg
The attached image is about the layout of the show all font link(including the hover style).
Attachment #8775088 - Attachment is obsolete: true
Attachment #8775094 - Attachment is obsolete: true
Attachment #8775420 - Attachment is obsolete: true
Attachment #8775088 - Flags: review?(ntim.bugs)
Attachment #8775892 - Flags: feedback?(hholmes)
Comment on attachment 8775088 [details]
Bug 1287422 - Don't overlap font-showall button and scrollbar in the Font Inspector.

https://reviewboard.mozilla.org/r/67404/#review65052

Nice use of theme-link!
Attachment #8775088 - Flags: review+
Comment on attachment 8775892 [details]
showall.jpeg

I agree with Tim, looks great!
Attachment #8775892 - Flags: feedback?(hholmes) → review+
Attachment #8775088 - Attachment is obsolete: false
Thanks for all the review and feedback!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/785efd442798
Don't overlap font-showall button and scrollbar in the Font Inspector. r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/785efd442798
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Are we still using the entity showAllFonts? If not, why wasn't this removed from the .dtd file?
Flags: needinfo?(schung)
(In reply to Francesco Lodolo [:flod] from comment #23)
> Are we still using the entity showAllFonts? If not, why wasn't this removed
> from the .dtd file?

We still leveraged the original longer string(showAllFonts) for the tooltip(see Attachment #8775892 [details]), so that's why the .dtd file still kept 2 different strings.
Flags: needinfo?(schung)
(In reply to Steve Chung [:steveck] from comment #24)
> We still leveraged the original longer string(showAllFonts) for the
> tooltip(see Attachment #8775892 [details]), so that's why the .dtd file
> still kept 2 different strings.

Thanks, not sure how I managed to miss the title="&showAllFonts;" in such a small landing.
I have reproduced this Bug on Nightly 50.0a1 (2016-07-18)  on Windows 10, 64 Bit!

The bug's fix is now verified on latest  Nightly 51.0a1 (2016-08-04)

Nightly 51.0a1:
Build ID 	20160804030441
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160803]
I have reproduced this bug with 50.0a1 (2016-07-18) on Ubuntu 14.04, 64 bit!

The bug's fix is now verified on latest Nightly 51.0a1.

Nightly 51.0a1:
Build ID 	20160804030441
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160803]
Attached image nodes.png
Verified that using the latest Nightly 51.0a1 2016-08-04 "Show all fonts used" link-like button is used under Mac OS X 10.9.5, Win 10 64-bit and Linux 14.04 64-bit.

Potential issues found:
- on OSX: font names are not recognized (" 
- select a node and scroll down on Fonts, then select another node and click on "Show all fonts used" - the scrollbar should be placed on top, instead the scrollbar is moved up with just a few pixels
- navigating between nodes makes their names disappear for a few seconds (see attachment)

Please review and let me know what should be filled next.
Flags: needinfo?(schung)
QA Contact: adalucin → cristian.comorasu
(In reply to Petruta Rasa [QA] [:petruta] from comment #28)
> Created attachment 8778275 [details]
> nodes.png
> 
> Verified that using the latest Nightly 51.0a1 2016-08-04 "Show all fonts
> used" link-like button is used under Mac OS X 10.9.5, Win 10 64-bit and
> Linux 14.04 64-bit.
> 
> Potential issues found:
> - on OSX: font names are not recognized (" 
Sorry I only have Linux platform, and I don't think this patch is sensitive to platform...

> - select a node and scroll down on Fonts, then select another node and click
> on "Show all fonts used" - the scrollbar should be placed on top, instead
> the scrollbar is moved up with just a few pixels
Is this that expected behavior originally? The position of the scrollbar seems unchanged previously. 

> - navigating between nodes makes their names disappear for a few seconds
> (see attachment)
I don't this it's related to this patch, and I can not reproduce this issue.
Flags: needinfo?(schung)
Regarding breadcrumbs, please see Bug 1185349.
Thanks Steve and Magicp for replying.

I checked again the first two potential issues, since the last one is already logged in bug 1185349.

The first one is a Firefox 44 regression, I'll file it today.

The second one is also reproducible in older build (eg Firefox 42) so I think this is expected.

Considering this and the verification made in comment 28, I'm marking this bug as verified.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
QA Contact: cristian.comorasu → petruta.rasa
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.