Closed Bug 1263709 Opened 8 years ago Closed 8 years ago

New WebExtension panel styles are sometimes a little problematic.

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48+ fixed, firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: bwinton, Assigned: bwinton)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(5 files)

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.
And here's what I've got so far.
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+
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)…
Whiteboard: triaged
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.
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)
Here you can see how style outputs in different browsers.
Flags: needinfo?(wachunei)
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
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!
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.
Attachment #8740474 - Flags: ui-review?(shorlander) → ui-review+
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.)
https://hg.mozilla.org/mozilla-central/rev/e50f2538de72
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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…
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?
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+
Attached image 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)
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)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.