[e10s] <option> elements don't inherit <select> font-size

NEW
Unassigned

Status

()

P3
normal
a year ago
6 months ago

People

(Reporter: kerberos0818, Unassigned)

Tracking

(Blocks: 2 bugs, {regression, stale-bug, testcase})

52 Branch
regression, stale-bug, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix)

Details

Attachments

(4 attachments)

(Reporter)

Description

a year ago
Created attachment 8850889 [details]
fire-fox-bug.png

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213829

Steps to reproduce:

add css for Select menu.
on nothing firefox addon.



Actual results:

1.but not enabled select menu css.
and firefox addon nothing.

2.specific enabled firefox addon.
enabled select menu css, font size changed bigger.




<!-- sample minimal HTML. -->
<!-- sample HTML start -->
<!DOCTYPE html>
<html>
<head>
</head>
<body>
<select style="font-size:90px;">
<option style="" value="red">red</option>
<option style="" value="green">green</option>
<option style="" value="blue">blue</option>
</select>
</body>
</html>

<!-- sample HTML end -->




****** specific  firefox addon example ********
****** enabled select menu css addon example ********
Make Link 
https://addons.mozilla.org/ja/firefox/addon/make-link/?src=userprofile

Drag & DropZones
https://addons.mozilla.org/ja/firefox/addon/drag-dropzones/

Image Stopper
https://addons.mozilla.org/ja/firefox/addon/image-stopper/

Tab Mix Plus
https://addons.mozilla.org/ja/firefox/addon/tab-mix-plus/





Expected results:

nothing firefox addon 
and 
select menu css enabled.

Comment 1

a year ago
bug 910022 seems to forget / ignore it.
Blocks: 1154677
Status: UNCONFIRMED → NEW
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
Component: Untriaged → Layout: Form Controls
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
Summary: firefox Addon is required for css activation on select menu → [e10s] font-size style for <option> elements

Updated

a year ago
Duplicate of this bug: 1314798
Duplicate of this bug: 1346002
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Too late for firefox 52, mass-wontfix.
status-firefox52: affected → wontfix
Summary: [e10s] font-size style for <option> elements → [e10s] font-size on <option> elements ignored
Summary: [e10s] font-size on <option> elements ignored → [e10s] <option> elements don't inherit <select> font-size
Too late for 53 since we are shipping next week. We could still possibly take a patch for 54 though.
status-firefox53: affected → wontfix
Jet, this seems like an unfortunate regression, is there any chance we could get this fixed for 54?
Flags: needinfo?(bugs)
(In reply to Nathan Froyd [:froydnj] from comment #7)
> Jet, this seems like an unfortunate regression
> is there any chance we could get this fixed for 54?

I don't know. There seems to be a number of regressions from e10s and <select> styling. There are performance concerns around pushing too many things over IPC to get 100% of the pre-e10s behavior back. Pushing over a font size doesn't sound like too much, but I don't know how much font data (e.g., for Web fonts) actually exists in the chrome process. 

jfkthame: any ideas on how to best fix this?
Flags: needinfo?(bugs) → needinfo?(jfkthame)
Presumably this issue applies to all the font-* properties (family, style, weight, etc), not just font-size.

If this were implemented at the C++ level in Gecko, I think it would be trivial to include a serialized nsFont record as part of the data we push to the chrome process to show a popup; that shouldn't be so much data that it materially impacts performance.

However, it looks like the implementation actually lives in toolkit JS-land (toolkit/modules/Select{Content,Parent}Helper.jsm) which I guess means we'd need to read the computed style of the items and add all the relevant font-* properties to the record, as was done for background and color in bug 910022. That seems likely to be substantially more expensive than if we were doing it at the C++ level by serializing and deserializing style structs, but I expect it'd still be OK.

Web fonts would remain an issue, as a webfont loaded by the child process won't be available in the parent. I believe we have the capability to fix that, based on the font serialization/deserialization stuff that Lee has been doing for printing via the chrome process, but it's possible that the amount of data we'd be pushing across might sometimes lead to a perceptible lag in popping up the menu. A large webfont (e.g. for CJK use) could be as much as a megabyte of data (although that's rare; most are in the range of a few tens of kilobytes, sometimes into the low hundreds). Also, I don't know if we currently have the ability to do this from front-end JS; we might need to add API to make that possible.

If we don't push the web font across, the result would be that the popup renders with whatever is next in the font-family list, which is the same behavior as the page itself would show if the webfont failed to load for any reason. That's probably reasonable as a first step, at least; as a followup, we could experiment with also serializing webfonts and see whether the performance is a problem.
Flags: needinfo?(jfkthame)
Created attachment 8860631 [details] [diff] [review]
(PoC) - Pass some basic font properties from content to parent process for <select> items

Something like this seems to basically work. Note that it doesn't currently pass all the font properties that might potentially be set (nor other things such as letter-spacing, text-transform, or text-decoration); I'm not sure how far it's worth taking this. Do we want to handle *every* CSS property that could be used to style each of the <select> items, or is that overkill?
I pushed a try job with the above patch, just to check if it'll disrupt any current tests.... so far (with a bunch still pending), looks OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7761e1ed7476469e671e361a25da99909e659552.
Created attachment 8876194 [details]
<select> test cases
font-size change could be easily perceived by users and unfortunately it's not working in Firefox.(Chrome, Edge, Opera OK, Safari NG)

Per comment 9, all font-* properties are probably not working correctly foe option text. I'd love to see we can fix them in order of importance.
Assignee: nobody → kuoe0
Status: NEW → ASSIGNED
Priority: -- → P1
See Also: → bug 1363152
I'm on PTO for the next week, but if someone wants to take over the patch from comment 10 and move forward with it, that's fine with me.
(In reply to Jonathan Kew (:jfkthame) [PTO until 2017-06-19] from comment #14)
> I'm on PTO for the next week, but if someone wants to take over the patch
> from comment 10 and move forward with it, that's fine with me.

Thanks, Jonathan. Tommy will follow up the fix.
Any updates on this bug, Tommy? :)
status-firefox54: affected → wontfix
status-firefox55: affected → fix-optional
status-firefox56: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(kuoe0)
Hi Ryan, I'll still fully support stylo in Q3. So, I didn't investigate this bug yet.
Flags: needinfo?(kuoe0)
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
status-firefox55: fix-optional → wontfix
status-firefox56: affected → wontfix
status-firefox57: --- → affected
I'll go back to work on this bug.

Updated

11 months ago
Blocks: 1404329
Duplicate of this bug: 1406351
Blocks: 1409613
No longer blocks: 1154677

Updated

10 months ago
Duplicate of this bug: 1409616

Updated

10 months ago
status-firefox57: affected → wontfix
Priority: P1 → P3
Comment hidden (off-topic)
Comment hidden (off-topic)
Status: ASSIGNED → NEW
Assignee: kuoe0.tw → nobody
You need to log in before you can comment on or make changes to this bug.