Closed
Bug 1499127
Opened 6 years ago
Closed 6 years ago
display:none on option/optgroup makes <select size=4> list have zero block-size
Categories
(Core :: Layout: Form Controls, defect, P4)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla64
| Tracking | Status | |
|---|---|---|
| firefox64 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: testcase)
Attachments
(2 files)
|
2.16 KB,
text/html
|
Details | |
|
7.69 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
All <select> scrollbars in the test should have the same height.
|
Assignee | |
Comment 1•6 years ago
|
||
The test PASS in Chrome and Edge, but it still fails in Firefox
due to bug 1499230. The block-size is correct now though.
Attachment #9017386 -
Flags: review?(emilio)
|
||
Comment 2•6 years ago
|
||
Comment on attachment 9017386 [details] [diff] [review]
fix+test
Review of attachment 9017386 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great, thanks!
::: layout/forms/nsListControlFrame.cpp
@@ +279,5 @@
> return a11y::eHTMLSelectListType;
> }
> #endif
>
> +// Return true if we found at least one <option> or none-empty <optgroup> label
nit: non-empty
@@ +281,5 @@
> #endif
>
> +// Return true if we found at least one <option> or none-empty <optgroup> label
> +// that has a frame. aResult will be the maximum BSize of those.
> +static bool
Maybe return Maybe<nscoord>? Though it's not a big deal in any case.
@@ +287,4 @@
> {
> + bool found = false;
> + for (nsIFrame* child : aContainer->PrincipalChildList()) {
> + if (HTMLOptGroupElement::FromNode(child->GetContent())) {
IsHTMLElement(nsGkAtoms::optgroup)?
@@ +297,3 @@
> } else {
> + // an option or optgroup label
> + bool isOptGroupLabel = child->Style()->IsPseudoElement() &&
We've already assumed that child->GetContent() is non-null above, so this test could also read child->GetContent()->IsGeneratedContentContainerForBefore() if you wanted to limit it to the before pseudo.
@@ +297,4 @@
> } else {
> + // an option or optgroup label
> + bool isOptGroupLabel = child->Style()->IsPseudoElement() &&
> + HTMLOptGroupElement::FromNode(aContainer->GetContent());
Maybe IsHTMLElement(nsGkAtoms::optgroup)?
::: testing/web-platform/tests/css/css-display/select-4-option-optgroup-display-none-ref.html
@@ +6,5 @@
> +<html><head>
> + <meta charset="utf-8">
> + <title>CSS Test: display:none on OPTION and OPTGROUP</title>
> + <link rel="author" title="Mats Palmgren" href="mailto:mats@mozilla.com">
> + <style type="text/css">
nit: no need for type="", here and on the other file.
::: testing/web-platform/tests/css/css-display/select-4-option-optgroup-display-none.html
@@ +28,5 @@
> +<optgroup class="none red">text</optgroup>
> +
> +<optgroup class="none red"><option>option</option></optgroup>
> +<optgroup><option class="none red">option</option></optgroup>
> +<optgroup class="contents red"><option class="none">option</option></optgroup>
This should probably not test display: contents until the relevant spec issues are sorted out, or at least maybe link to https://github.com/whatwg/html/issues/4093 in a comment?
Though looking at it more carefully it looks like it'd past regardless of what the decision there ends up being, so nvm.
Attachment #9017386 -
Flags: review?(emilio) → review+
|
|
Assignee | |
Comment 3•6 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
> > +static bool
>
> Maybe return Maybe<nscoord>?
I rewrote it using Maybe<nscoord> instead, but I found it made
the code harder to read, so I kept the outparam version.
> > + if (HTMLOptGroupElement::FromNode(child->GetContent())) {
>
> IsHTMLElement(nsGkAtoms::optgroup)?
OK
> > + bool isOptGroupLabel = child->Style()->IsPseudoElement() &&
>
> We've already assumed that child->GetContent() is non-null above, so this
> test could also read
> child->GetContent()->IsGeneratedContentContainerForBefore() if you wanted to
> limit it to the before pseudo.
It doesn't matter much - I expect this code to be removed when
we fix bug 1499176.
> > + HTMLOptGroupElement::FromNode(aContainer->GetContent());
>
> Maybe IsHTMLElement(nsGkAtoms::optgroup)?
OK
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abaa52cda0ad
Only consider <option>/<optgroup> elements that has a frame when determining the default row size, and use a fallback value if there are none. r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13545 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
|
||
Comment 7•6 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Upstream PR merged
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13545
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/WSGSCQx5TViIaIJroy99ZA)
|
||
Updated•6 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•