Closed Bug 1565652 Opened 1 year ago Closed 1 year ago

[Inactive CSS] Wrong warning about height and display on replaced elements.

Categories

(DevTools :: Inspector: Rules, defect, P2)

defect

Tracking

(firefox69 verified, firefox70 verified)

VERIFIED FIXED
Firefox 70
Tracking Status
firefox69 --- verified
firefox70 --- verified

People

(Reporter: emilio, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

  • Open data:text/html,<select style="display: inline; height: 300px"><option>Foo</option></select>
  • See a 300px tall <select> element.
  • Open devtools, inspect the <select>
  • See how it claims the rule to have no effect, but it does.
Blocks: 1540753
Priority: -- → P2
Summary: The unused css stuff is wrong about height and display on replaced elements. → [Inactive CSS] Wrong warning about height and display on replaced elements.

I was talking to Emilio now, so pasting something super useful he said:

Form controls are weird... They generally are replaced elements, except when they're not. <select> is "replaced" if it has size<=1 or no size specified.
Checkboxes and radios are replaced unless they have -moz-appearance: none:
So data:text/html,<input type="checkbox" style="width: 300px; height: 300px; display: inline;"> works
But data:text/html,<input type="checkbox" style="width: 300px; height: 300px; display: inline; -moz-appearance: none"> doesn't

Assignee: nobody → pbrosset
Status: NEW → ASSIGNED

Before this patch we would only treat audio elements as replaced if they
had something visible on the page, so if they had the controls attribute.
This is a specific case that we don't really need to worry about. If we
unconditionnally assume audio elements are replaced, then the code is
simpler and the heuristic is still fine for the vast majority of cases.
In fact, it's even more correct, as an audio element that's inline and
does not have the controls attribute still has active width/height
properties. So we do need to treat it as replaced even in this case.

Try push for the first patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72b4b62204f1346ccf62980028fbe1ccd23a74c6&group_state=expanded
The second patch is a really simple change and there is no specific test case just for the audio tag, so I've not pushed that one to try.

Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/457f5f4f3c97
Treat input and select unconditionally as replaced elements; r=miker
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/90c229e681a3
Treat all audio elements as replaced; r=miker

Just noticed, missed textarea? I was debugging some textarea-related code in bug 1569078 and saw the warning :)

Flags: needinfo?(pbrosset)
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Thanks Emilio, I'll file a new bug for this.

Flags: needinfo?(pbrosset)
See Also: → 1569877

Confirmed the issue with 69.0b8.
Fix verified with 70.0a1 (2019-07-29).

Status: RESOLVED → VERIFIED

Comment on attachment 9080337 [details]
Bug 1565652 - Treat input and select unconditionally as replaced elements; r?miker!

Beta/Release Uplift Approval Request

  • User impact if declined: Inactive CSS is a new DevTools feature we are planning on shipping to all our users with Firefox 69.
    It is undergoing QA validation in beta 69 right now.
    This bug was identified recently and impacts negatively the user experience for this new feature.

If we do not uplift this bug fix, some users of the Inspector panel in DevTools might be shown incorrect information as to when their CSS code is valid or not.
This will have negative impact on the confidence they have in the feature and may not want to use it afterwards.

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: Manual validation by QE already occurred.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It is not risky because the code change is very small and contained to just the Rule-View sidebar of the Inspector panel in DevTools.
  • String changes made/needed:
Attachment #9080337 - Flags: approval-mozilla-beta?
Attachment #9080361 - Flags: approval-mozilla-beta?

Comment on attachment 9080337 [details]
Bug 1565652 - Treat input and select unconditionally as replaced elements; r?miker!

Fixes a serious bug in a new devtools feature shipping in 69. Approved for 69.0b10.

Attachment #9080337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9080361 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified with 69.0b10(build from taskcluster) on Windows 10, macOS 10.14, Ubuntu 16.04.

You need to log in before you can comment on or make changes to this bug.