New WebExtension panel styles are sometimes a little problematic.

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: bwinton, Assigned: bwinton)

Tracking

({dev-doc-complete})

unspecified
mozilla49
dev-doc-complete

Firefox Tracking Flags

(firefox48+ fixed, firefox49 fixed)

Details

(Whiteboard: triaged)

Attachments

(5 attachments)

Created attachment 8740112 [details]
A screenshot of the problem.

Some people have all their buttons together, and all the labels after them, but would like to not show the button for the last label.  Patch written, screenshots coming.
(Assignee)

Comment 1

3 years ago
Created attachment 8740456 [details]
A screenshot of the current code.

And here's what I've got so far.
(Assignee)

Comment 2

3 years ago
Created attachment 8740474 [details]
MozReview Request: Bug 1263709 - Remove the advanced styling for radio buttons and checkboxes.  ui-r=shorlander, r=kmag

Review commit: https://reviewboard.mozilla.org/r/45787/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45787/
Attachment #8740474 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

3 years ago
Attachment #8740474 - Flags: ui-review?(shorlander)
Comment on attachment 8740474 [details]
MozReview Request: Bug 1263709 - Remove the advanced styling for radio buttons and checkboxes.  ui-r=shorlander, r=kmag

https://reviewboard.mozilla.org/r/45787/#review42431

::: browser/components/extensions/extension.css
(Diff revision 1)
> -input[type="radio"]:checked:hover:active + label::before,
> -.radioItem.pressed input[type="radio"]:checked:not(active) + label::before {
> -  background-color: #005bab;
> -  border-color: #004480;
> -}
> -

Maybe we should add a class so pages that want these styles can opt into them?
Attachment #8740474 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 4

3 years ago
I kinda waffled back and forth on that.  I eventually went this way because I think we want most WebExtensions to just look good, which implies getting the nicer styles by default.  (It also makes it easier to port existing WebExtensions to Firefox.)  But I obviously agree to a certain point, since there are some styles which depend on classes (like panel-list-item or panel-section-footer)…

Updated

3 years ago
Whiteboard: triaged

Comment 5

3 years ago
Can it be an option on the manifest? I understand the need to set an UI standard as default, but it would be nice for cross-browser extension to opt-out of it to keep the same style between browsers.
(Assignee)

Comment 6

3 years ago
Hi Pedro!  What do you think about using a CSS reset (as described at http://meyerweb.com/eric/tools/css/reset/)?  Also, (assuming you're writing an add-on) how does it look in Chrome vs. Firefox?

(It's not going to be an option on the manifest, but I think we should be able to work out some way to disable it.)
Flags: needinfo?(wachunei)

Comment 7

3 years ago
Created attachment 8744616 [details]
example extension in different browsers

Here you can see how style outputs in different browsers.
Flags: needinfo?(wachunei)

Comment 8

3 years ago
I think it is okay to use a CSS reset when you pretend to override the default style, it is the best option when you want to do that.

The point here is that CSS rules are not being applied correctly, my extension is not overriding default styles (you can see that in the options page of Chrome and Opera, select tag is styled according to browser CSS), but I'm using CSS to hide radio inputs (positioning them off viewport), which is working fine in other browsers (and Firefox too) but not working on the first option (SIDING).

Here's the CSS rule I'm using to hide those: https://github.com/wachunei/directUC/blob/master/css/popup.css#L141
(Assignee)

Comment 9

3 years ago
So, the radio input is exactly what this bug is fixing, as you can see in my screenshot from comment #1.  Hopefully it'll land soon!

Comment 10

3 years ago
You are right, but at the same time it is removing your UI styles for those inputs, when the issue might be a CSS specificity problem.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #6)
> It's not going to be an option on the manifest, but I think we should be
> able to work out some way to disable it.

I actually think we should probably add a "browser_style" option to match the one in "options_ui".
It would be great if we could land this before the 48 merge. Otherwise I think we're going to have to uplift it.
kmag: I agree!  I got a verbal ui-r+ from shorlander this week, so if he doesn't set the flag tomorrow, I think we would be fine landing it.
https://reviewboard.mozilla.org/r/45787/#review42431

> Maybe we should add a class so pages that want these styles can opt into them?

(As mentioned in bugzilla, I think we want styles to apply by default, to make everyone look better.)

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e50f2538de72
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
[Tracking Requested - why for this release]: This makes the styles nicer for add-on authors for the first release of WebExtensions.  It's a CSS-only change, and so has little chance of breaking things, and we don't want add-on authors to be frustrated with us, or have to work around this bug for the first release…
tracking-firefox48: --- → ?
Comment on attachment 8740474 [details]
MozReview Request: Bug 1263709 - Remove the advanced styling for radio buttons and checkboxes.  ui-r=shorlander, r=kmag

Approval Request Comment
[Feature/regressing bug #]: Fix for problems with bug 1225633.
[User impact if declined]: WebExtensions authors will need to work around our styles, and be annoyed by it.
[Describe test coverage new/current, TreeHerder]: Manual, style-related.
[Risks and why]: Low-risk because it's only removing CSS.
[String/UUID change made/needed]: None.
Attachment #8740474 - Flags: approval-mozilla-beta?
Comment on attachment 8740474 [details]
MozReview Request: Bug 1263709 - Remove the advanced styling for radio buttons and checkboxes.  ui-r=shorlander, r=kmag

(Dammit, I should stop doing things before coffee!)
Attachment #8740474 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
status-firefox48: --- → affected
Comment on attachment 8740474 [details]
MozReview Request: Bug 1263709 - Remove the advanced styling for radio buttons and checkboxes.  ui-r=shorlander, r=kmag

Styling usage improvements in webextensions, CSS only so low risk, Aurora48+
Attachment #8740474 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox48: ? → +

Comment 21

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5eab0f59fd9e
status-firefox48: affected → fixed
Created attachment 8754341 [details]
3d.jpg

I’ve intended to verify this bug using the following xpi https://bugzilla.mozilla.org/show_bug.cgi?id=1225633#c35 but it seems to be broken (see the attached screenshot) since Bug 1269081 has landed.

Is this webextension outdated and needs to be updated according to Bug 1269081 or this is a regression that require a new bug?

Blake, could you please provide me also the other webextension (from the first attachment)?
Flags: needinfo?(bwinton)
Hey Vasilica,

the other add-on is at https://github.com/wachunei/directUC (You can download a zip file of it at https://github.com/wachunei/directUC/archive/master.zip, then unzip it, go to "about:debugging", and use the "Load Temporary Add-on" button to load the unzipped directory, by picking any of the files in that directory.)

I haven't updated the UI Playground to take advantage of the brower_style yet, so what you're seeing is expected behaviour.  I'll probably get around to updating it soon, maybe, but if you need it for testing, I could update it earlier for you.

Thanks,
Blake.
Flags: needinfo?(bwinton)
Keywords: dev-doc-needed
I've added something on this here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/page_action#Syntax
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Syntax

and also:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Anatomy_of_a_WebExtension#Popups

Do we need anything else?

I'm also wondering, what's the cross-browser story for this? If the CSS classes described here: https://firefoxux.github.io/StyleGuide/#/components/panels are essentially part of the public API, do we expect other browsers to support them, and to apply styles for their own browsers using them? If so, should we have cross-browser docs for them?
Flags: needinfo?(bwinton)
That all looks pretty good.  I'm not sure about cross-browser support.  That sounds like a question for the people writing the spec…  ;)
Flags: needinfo?(bwinton)
Keywords: dev-doc-needed → dev-doc-complete

Updated

8 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.