Closed Bug 1400831 Opened 7 years ago Closed 7 years ago

tabbar size increase web drag link over (compact theme only)

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Version:
57 Branch
Platform:
x86
macOS
Type:
defect

Tracking

()

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

People

(Reporter: cpartiot, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(4 files)

tabbar height gets increased by tab drop indicator
16.63 KB, image/gif
Details
Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS.
59 bytes, text/x-review-board-request
dao
: review+
ritu
: approval-mozilla-beta+
Details
shorterTabIndicator-screencast.gif
541.03 KB, image/gif
Details
illustration of broken shadow
3.38 KB, image/png
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170917220255

Steps to reproduce:

- set compact theme
- drag any link on tab bar (between two tabs)


Actual results:

tabbar height change


Expected results:

tabbar height should not change
WFM in 57 20170917220255 on Win10.
Component: Untriaged → Theme
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86
reproduced on 57.0a1 (2017-09-18) (64-bit), clean profile, on macOS, retina display.

the tab drop indicator (.tab-drop-indicator) is too tall for compact tab bar.
https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/browser/themes/osx/browser.css#924-935
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: photon-tabs
Flags: qe-verify+
Priority: -- → P4
Whiteboard: [reserve-photon-visual]
QA Contact: ovidiu.boca
Priority: P4 → P3
I can't reproduce this on OSX, on latest nightly. In either the default theme or a compact theme? Can you still see this :arai?
Flags: needinfo?(arai.unmht)
I can reproduce it on 58.0a1 (2017-10-01) (64-bit) on macOS with clean profile, set to Compact density,
also on non-retina display.
Flags: needinfo?(arai.unmht)
(In reply to Tooru Fujisawa [:arai] from comment #6)
> I can reproduce it on 58.0a1 (2017-10-01) (64-bit) on macOS with clean
> profile, set to Compact density,
> also on non-retina display.

Ah, I was missing the compact density. Thanks. I'll look into this.
Assignee: nobody → sfoster
Not sure if we have an SVG for this image, or if there was a reason it wasnt replaced with the other icons. That might improve things here particularly for non-retina display where any scaling will be more noticable. Seems like using the --tab-min-height variable is the right thing to do here though?
Status: NEW → ASSIGNED
Priority: P3 → P1
(In reply to Sam Foster [:sfoster] from comment #9)
> Not sure if we have an SVG for this image, or if there was a reason it wasnt
> replaced with the other icons. That might improve things here particularly
> for non-retina display where any scaling will be more noticable. Seems like
> using the --tab-min-height variable is the right thing to do here though?

I don't think we should scale this image but just make the image a bit shorter (i.e. remove a pixel few rows). Should be easy to do with Gimp or some other graphics editor. Switching to an SVG image would be even better but not required.
Comment on attachment 8914435 [details]
Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS.

https://reviewboard.mozilla.org/r/185738/#review190982
Attachment #8914435 - Flags: review?(dao+bmo)
https://cl.ly/3c2b3D3t1n26 shows the shorter indicator in use on a default/normal density theme. It looked funny aligned to bottom edge, so I vertically centered it.
Vertically centered, shorter indicator on normal density theme. On a compact density theme the image fits vertically.
Attachment #8914866 - Flags: ui-review?(shorlander)
Comment on attachment 8914435 [details]
Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS.

https://reviewboard.mozilla.org/r/185738/#review191330

::: browser/base/content/tabbrowser.xml:6259
(Diff revision 3)
>      <resources>
>        <stylesheet src="chrome://browser/content/tabbrowser.css"/>
>      </resources>
>  
>      <content>
> -      <xul:hbox align="end">
> +      <xul:hbox align="center">

This is wrong on Windows and Linux. You could add a class here and set -moz-box-align in the theme stylesheets.
Attachment #8914435 - Flags: review?(dao+bmo) → review-
indicator is ugly on dark mode

https://imgur.com/a/GZwzY
(In reply to Dão Gottwald [::dao] from comment #16)
> Comment on attachment 8914435 [details]
> Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and
> compact themes.
> 
> https://reviewboard.mozilla.org/r/185738/#review191330
> 
> ::: browser/base/content/tabbrowser.xml:6259
> (Diff revision 3)
> >      <resources>
> >        <stylesheet src="chrome://browser/content/tabbrowser.css"/>
> >      </resources>
> >  
> >      <content>
> > -      <xul:hbox align="end">
> > +      <xul:hbox align="center">
> 
> This is wrong on Windows and Linux. You could add a class here and set
> -moz-box-align in the theme stylesheets.

Or you could make the indicator image consistent across platforms.
(In reply to Clément from comment #17)
> indicator is ugly on dark mode
> 
> https://imgur.com/a/GZwzY

I'm not sure what I'm looking for in that image? The shadow shouldnt be changed in this patch, so unless I messed something up editing and re-optimizing this png, this is an existing issue and should be filed separately.
(In reply to Dão Gottwald [::dao] from comment #18)

> Or you could make the indicator image consistent across platforms.

That sounds like a can of worms. At least, I don't know the history and reasoning around why we have different images. Perhaps for no good reason, but probably that would need to be a different bug. 

Meantime, I've moved the box-align to CSS in tabbrowser.css, and an override in the OSX stylesheet to center it.
(In reply to Sam Foster [:sfoster] from comment #22)
> (In reply to Dão Gottwald [::dao] from comment #18)
> 
> > Or you could make the indicator image consistent across platforms.
> 
> That sounds like a can of worms. At least, I don't know the history and
> reasoning around why we have different images. Perhaps for no good reason,
> but probably that would need to be a different bug. 

There's no compelling reason for these differences. The images were created at different times by different people.
Comment on attachment 8914435 [details]
Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS.

https://reviewboard.mozilla.org/r/185738/#review191756

The images are a bit messed up -- the shadow seems to overlap on the sides. Please try cropping them more carefully, or ask somebody from UX to help out, or let me know if you want me to do it.
Attachment #8914435 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #24)
> The images are a bit messed up -- the shadow seems to overlap on the sides.
> Please try cropping them more carefully, or ask somebody from UX to help
> out, or let me know if you want me to do it.

All I did here was cut out 4 (8 for the @2x) pixels from the middle and have not touched the shadows at all. But gimp added some bloat so had run them through pngcursh. It seems it was a bit over-zealous and I didn't spot the problem on my thinkpad display. I re-did the image edits with pixelmator and it seems to have done a better job while not adding bloat in the process.
still not right...
Comment on attachment 8914435 [details]
Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS.

https://reviewboard.mozilla.org/r/185738/#review192172
Attachment #8914435 - Flags: review?(dao+bmo) → review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1a3a89200b7
Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS. r=dao
https://hg.mozilla.org/mozilla-central/rev/e1a3a89200b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8914435 [details]
Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS.

Approval Request Comment
[Feature/Bug causing the regression]: photon tabs
[User impact if declined]: tab strip grows vertically when dragging a link there in compact mode
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: basically just making the drop indicator icon shorter to fit the compact tab strip
[String changes made/needed]: /
Attachment #8914435 - Flags: approval-mozilla-beta?
Comment on attachment 8914866 [details]
shorterTabIndicator-screencast.gif

UI review moot, and probably not necessary in the first place
Attachment #8914866 - Flags: ui-review?(shorlander)
Comment on attachment 8914435 [details]
Bug 1400831 - Use a shorter tab-drop-indicator image to fit both default and compact themes, and move its box-align to CSS.

Photon related, Beta57+
Attachment #8914435 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I verified this issue using Latest Nightly 58.0a1 with Build ID 20171006100327 on Mac OS X 10.12.
I will mark this as verified fixed.
status-firefox58: fixed → verified
Status: RESOLVED → VERIFIED
I managed to reproduce this bug using an older version of Nightly (2017-09-20) on macOS 10.12 and Mac OS X 10.11.
I retested everything using the latest Nightly 58 and beta 57.0b8 on the same platforms and the bug is not reproducing anymore.
status-firefox57: fixed → verified
Flags: qe-verify+
See Also: → 1449366
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: