Tour panels/bubbles have uneven padding dimensions

RESOLVED FIXED in Firefox 48

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: phlsa, Assigned: Unfocused, Mentored)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 48
Unspecified
Windows
Points:
5
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(3 attachments, 2 obsolete attachments)

Posted 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)
Posted 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)
Posted 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]
Points: --- → 5
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attachment #8643012 - Attachment is obsolete: true
Attachment #8643265 - Attachment is obsolete: true
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).
Posted 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.

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1306c789088d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.