Closed Bug 1180186 Opened 5 years ago Closed 4 years ago

Tour panels/bubbles have uneven padding dimensions

Categories

(Firefox :: Theme, defect, P3)

Unspecified
Windows
defect
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: phlsa, Assigned: Unfocused, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qx][outreachy-12])

Attachments

(3 files, 2 obsolete files)

Attached image Panel mockup
On Windows, the top padding of tour panels is significantly higher than the bottom padding. Since I assume that making the top padding smaller would cause overlap issues with the close button, let's increase the bottom padding instead.

Mockup is attached.

(Matt, I think this might be relevant to TP onboarding)
Flags: needinfo?(MattN+bmo)
Attached patch fix-tour-bubble-margins (obsolete) — Splinter Review
This should fix it.
Tested on Windows 7 and Mac (to make sure it doesn't screw things up there).
Attachment #8643012 - Flags: review?(MattN+bmo)
Attached image Screen Shot of attachment 8643012 (obsolete) —
This doesn't look right to me. See screen shot.
Flags: needinfo?(MattN+bmo)
Attachment #8643012 - Flags: review?(MattN+bmo) → review-
Hm, do you have an idea how I can target panels that don't have such a footer?
I guess I could also try moving the bottom spacing (currently a margin-top on the footer) to the body.

Are there any other occurances of the panel that I should watch out for besides the panel in your screen shot and the reader mode bubble?
Flags: needinfo?(MattN+bmo)
You can check all of the tours (Hello, Yahoo, whatsnew, etc.) but I think the footer for buttons/links in the only variable affecting vertical layout. Note that the image on the left is also optional and affects horizontal layout.
Flags: needinfo?(MattN+bmo)
Mentor: jaws
Whiteboard: [qx] → [qx][outreachy-12]
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attachment #8643012 - Attachment is obsolete: true
Attachment #8643265 - Attachment is obsolete: true
Attached image whitespace comparison
I think adding even more padding to the bottom results in too much whitespace overall in the panel - it becomes out of balance with the actual content (see screenshot, top half of the image). So alternatively, I've worked around the issue of the title overlapping with the close button and removed whitespace from above the content (see screenshot, bottom half of the image).
Attached patch Patch v2Splinter Review
Oh, and as a bonus, this ends up making this closer to other panels we have (like the identity panel), which have less whitespace.

Also ended up simplifying some of the CSS here.
Attachment #8739931 - Flags: review?(MattN+bmo)
Comment on attachment 8739931 [details] [diff] [review]
Patch v2

(Forgot MattN is swamped with e10s/password manager stuff)
Attachment #8739931 - Flags: review?(MattN+bmo) → review?(jaws)
Comment on attachment 8739931 [details] [diff] [review]
Patch v2

Review of attachment 8739931 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/UITour.inc.css
@@ +53,1 @@
>    margin: 0 0 9px 0;

The -moz-margin-start and -moz-margin-end rules here are being overwritten immediately by the margin:0 0 9px 0; rule here. You can just change the `margin` rule to specify `margin-top: 0;` and `margin-bottom: 9px;`.

Also, you should set `position: relative;` on #UITourTooltipClose so the close button is on a higher layer than #UITourTooltipBody. Without `position: relative;` the cursor doesn't get the hover state correct on the left-most 12 or so pixels. See this screencast for example: http://screencast.com/t/861ImaI5
Attachment #8739931 - Flags: review?(jaws) → review-
Comment on attachment 8739931 [details] [diff] [review]
Patch v2

r+ with the previous comments addressed.
Attachment #8739931 - Flags: review- → review+
(In reply to Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) from comment #7)
> (Forgot MattN is swamped with e10s/password manager stuff)

Thanks for moving this over to Jared as I do have a backlog.

I just wanted to remind you that the buttons and image are optional (which some people forget) so please ensure that this doesn't regress when they aren't specified.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> The -moz-margin-start and -moz-margin-end rules here are being overwritten

*facepalm* Done.
 
> Also, you should set `position: relative;` on #UITourTooltipClose

Dunno how I missed that - done.


(In reply to Matthew N. [:MattN] (behind on reviews) from comment #10)
> I just wanted to remind you that the buttons and image are optional (which
> some people forget) so please ensure that this doesn't regress when they
> aren't specified.

Yep! I'd spent some time testing the various combinations, on all OSes.
https://hg.mozilla.org/mozilla-central/rev/1306c789088d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1270395
You need to log in before you can comment on or make changes to this bug.