Closed Bug 1499127 Opened Last year Closed Last year

display:none on option/optgroup makes <select size=4> list have zero block-size

Categories

(Core :: Layout: Form Controls, defect, P4, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files)

Attached file Testcase
All <select> scrollbars in the test should have the same height.
Blocks: 1499184
Blocks: 1499230
Attached patch fix+testSplinter Review
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 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+
(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.
https://hg.mozilla.org/mozilla-central/rev/abaa52cda0ad
Status: NEW → RESOLVED
Closed: Last year
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)
You need to log in before you can comment on or make changes to this bug.