permission states wrapped on multiple lines don't align on the end (right for ltr)

RESOLVED FIXED in Firefox 55

Status

()

P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aryx, Assigned: prathiksha, Mentored)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox53 wontfix, firefox54 fix-optional, firefox55 fixed)

Details

(Whiteboard: [fxprivacy])

Attachments

(3 attachments)

Posted image screenshot of issue
Tested with Firefox 54.0a1 and 53.0a2 on Windows 8.1

If a permission state in the permission list wraps, the end of the lines don't align because there is a css rule with white-space: pre-wrap; - this is not needed because the permissions are in <hbox>es (but needed e.g. for showing linebreaks in certificate information).

Steps to reproduce:
1. Open https://developer.mozilla.org/en-US/docs/Web/API/notification
2. Uncheck the box to remember the decision in the doorhanger asking for the permission to show notifications.
3. Deny the permission.
4. Click on the blocked permission icon.

This doesn't affect English (screenshot of the issue is how German will look like) but you can add longer strings using the Browser Toolbox.

Comment 2

2 years ago
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #0)
> (but needed e.g. for showing linebreaks in certificate information).

Looks like we should only apply pre-wrap to the description elements(s) where we use line breaks?

Comment 3

2 years ago
Comment on attachment 8841362 [details]
Bug 1342793 - control center: permission states wrapped on multiple lines should align on the end.

Sorry, I meant comment 2 to be addressed before going forward with the review...
Attachment #8841362 - Flags: review?(paolo.mozmail)
Aryx, would you have time to finish this up anytime soon? The patch seems simple enough to uplift and we might still make it for 53.

Thanks! :)

> Looks like we should only apply pre-wrap to the description elements(s) where we use line breaks?

Maybe I'm misunderstanding but how do you imagine that to work in CSS?
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
Flags: needinfo?(aryx.bugmail)
Priority: -- → P3
Whiteboard: [fxprivacy]

Comment 5

2 years ago
(In reply to Johann Hofmann [:johannh] from comment #4)
> > Looks like we should only apply pre-wrap to the description elements(s) where we use line breaks?
> 
> Maybe I'm misunderstanding but how do you imagine that to work in CSS?

With the current patch, these elements still get the pre-wrap:

#identity-popup-security-content > description,
#identity-popup-security-descriptions > description,
#identity-popup-securityView-header > description,
#identity-popup-securityView-body > description,
#identity-popup-permissions-content > description,

I think some of them may not need it.

It may be better to remove the pre-wrap from the current combined rule, so that only "font-size" and "margin" would apply to all these elements, and then add a separate rule to apply pre-wrap only to the elements that need it.
Ok, gotcha, thanks for the clarification. That makes absolute sense.
Aryx, I'm unassigning you from this for now, let me know if you have time to pick it up again.

I'm making this a mentored bug, happy to help if anyone wants to finish this up.
Assignee: aryx.bugmail → nobody
Mentor: jhofmann
Status: ASSIGNED → NEW
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional
status-firefox55: affected → fix-optional
Flags: needinfo?(aryx.bugmail)
Priority: P3 → P5
(Assignee)

Updated

2 years ago
Assignee: nobody → prathikshaprasadsuman

Comment 9

2 years ago
mozreview-review
Comment on attachment 8867530 [details]
Bug 1342793 - Style the permission states wrapped on multiple lines in the control center to align correctly.

https://reviewboard.mozilla.org/r/139080/#review142476

Thanks for picking this up! I think this patch isn't exactly what Paolo meant, he wanted to share the font-size and margin rules between all elements and only apply "white-space: pre-wrap" where it's really needed.

Come to think of it, is it actually needed on any of these elements? I don't think we have explicit line breaks in any of our identity popup copy. I'd recommend simply removing the white-space: pre-wrap; line and flagging Paolo for review. If he can point you to an element that needs it you can just add another rule for that. Otherwise I think it's fine to just get rid of it. :)
Attachment #8867530 - Flags: review?(jhofmann)
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8867530 [details]
Bug 1342793 - Style the permission states wrapped on multiple lines in the control center to align correctly.

https://reviewboard.mozilla.org/r/139080/#review142476

Okay. I'll do that. :)
Comment hidden (mozreview-request)

Comment 12

2 years ago
I don't know if some elements still need the pre-wrap off the top of my head, I'll have to research this.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8867530 [details]
Bug 1342793 - Style the permission states wrapped on multiple lines in the control center to align correctly.

https://reviewboard.mozilla.org/r/139080/#review147796

Sorry for the delay here... I think "identity-popup-content-supplemental" still needs the pre-wrap because we add newlines to it in the code. I believe you can test this by opening the security subview on the bugzilla.mozilla.org website itself.

While other fields in the certificate may, as far as I know, embed newlines or have multiple spaces in some places, I don't think it's an issue if we don't wrap them or don't display the multiple spaces, so the one above is the only element that needs the rule.

Let's add a comment to the new CSS rule to explain this.
Attachment #8867530 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)

Comment 15

2 years ago
Comment on attachment 8867530 [details]
Bug 1342793 - Style the permission states wrapped on multiple lines in the control center to align correctly.

Looks good, thank you!
Attachment #8867530 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 years ago
mozreview-review
Comment on attachment 8867530 [details]
Bug 1342793 - Style the permission states wrapped on multiple lines in the control center to align correctly.

https://reviewboard.mozilla.org/r/139080/#review150598
Attachment #8867530 - Flags: review?(paolo.mozmail) → review+

Comment 18

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1cfed9c2d4c2 -d 1df01865d100: rebasing 400460:1cfed9c2d4c2 "Bug 1342793 - Style the permission states wrapped on multiple lines in the control center to align correctly. r=Paolo" (tip)
merging browser/themes/shared/controlcenter/panel.inc.css
warning: conflicts while merging browser/themes/shared/controlcenter/panel.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Updated

2 years ago
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed

Comment 20

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d9b245a92c6c
Style the permission states wrapped on multiple lines in the control center to align correctly. r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d9b245a92c6c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.