Closed Bug 1342793 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Site Identity, defect, P5)

Product:
Firefox
Component:
Site Identity
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox53 --- wontfix
firefox54 --- fix-optional
firefox55 --- fixed

People

(Reporter: aryx, Assigned: prathiksha, Mentored)

Details

(Whiteboard: [fxprivacy])

Attachments

(3 files)

screenshot of issue
14.99 KB, image/png
Details
Bug 1342793 - control center: permission states wrapped on multiple lines should align on the end.
59 bytes, text/x-review-board-request
Details
Bug 1342793 - Style the permission states wrapped on multiple lines in the control center to align correctly.
59 bytes, text/x-review-board-request
Paolo
: review+
Details
Attached 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.
(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 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]
(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: nobody → prathikshaprasadsuman
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)
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. :)
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 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 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+
Keywords: checkin-needed
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+
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)
Flags: needinfo?(prathikshaprasadsuman)
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
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
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: