1583902 - [Inactive CSS] Display a warning when 'block-overflow' etc. are used on non-block containers

Status: RESOLVED FIXED

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

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Main file:
devtools/server/actors/utils/inactive-property-helper.js

invalidProperties: [
"block-overflow",
"columns",
"column-count",
"column-width",
"hyphenate-limit-lines",
"hyphenate-limit-last",
"hyphenate-limit-zone",
"leading-trim",
"leading-trim-over",
"leading-trim-under",
"line-clamp",
"line-grid",
"tab-size",
"text-align",
"text-align-all",
"text-align-last",
"text-group-align",
"text-indent",
]

inactive-css-only-block-containers = <strong>{ $property }</strong> has no effect on this element since it can only be applied to block containers.

inactive-css-only-block-containers-fix = Try adding <strong>display:block</strong>. { learn-more }

Comment 1

[Daniel Holbert [:dholbert]]

We've got a sizeable list of properties in comment 0. Some of these properties are inherited and should be excluded from any sort of warning-rule that we might add here.

e.g. tab-size, text-align and text-indent are inherited-by-default, according to mdn[0][1][2] (On the other hand, column-count and column-width are not inherited and would be potentially fine to warn about.)

For the inherited-by-default properties like text-align and text-indent, we should not add any sort of "inactive css" warnings, per my notes in bug 1583901 comment 9 onwards, and the code-comment added in bug 1688538.

[0] https://developer.mozilla.org/en-US/docs/Web/CSS/tab-size
[1] https://developer.mozilla.org/en-US/docs/Web/CSS/text-align
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/text-indent

Comment 2

[Sebastian Zartner [:sebo]]

I took the time to go through all specs that have properties that apply to block containers. Here's a table with the related info:

Name	Applies to	Inherited	Specs
align-content	block containers, multicol containers, flex containers, and grid containers	no	CSS Align 3
block-ellipsis	block containers	yes	CSS Overflow 4
column-count	block containers except table wrapper boxes	no	CSS Multi-column 1
column-width	block containers except table wrapper boxes	no	CSS Multi-column 1
columns	block containers except table wrapper boxes	no	CSS Multi-column 1
continue	block containers and multicol containers	no	CSS Overflow 4
continue	block containers, flex containers, and grid containers	no	CSS Overflow 5
dominant-baseline	block containers, inline boxes, table rows, grid containers, flex containers, and SVG text content elements	yes	CSS Inline 3
flow-from	Non-replaced block containers.	no	CSS Regions 1
hyphenate-limit-zone	block containers	yes	CSS Text 4
hyphenate-limit-lines	block containers	yes	CSS Text 4
hyphenate-limit-last	block containers	yes	CSS Text 4
line-height-step	block containers	yes	CSS Rhythm 1
margin-trim	block containers, multi-column containers, flex containers, grid containers	no	CSS Box 4
max-lines	block containers which are also fragmentation containers that capture region breaks	no	CSS Overflow 4
orphans	block containers that establish an inline formatting context	yes	CSS Break 3, CSS Break 4
overflow	block containers	no	CSS 2.1
overflow	block containers, flex containers, grid containers	no	CSS Overflow 3
overflow-block	block containers, flex containers, grid containers	no	CSS Overflow 3
overflow-inline	block containers, flex containers, grid containers	no	CSS Overflow 3
overflow-x	block containers, flex containers, grid containers	no	CSS Overflow 3
overflow-y	block containers, flex containers, grid containers	no	CSS Overflow 3
place-content	block containers, flex containers, and grid containers	no	CSS Align 3
text-align	block containers	yes	CSS Text 3, CSS Text 4, CSS 2.1
text-align-all	block containers	yes	CSS Text 3, CSS Text 4
text-align-last	block containers	yes	CSS Text 3, CSS Text 4
text-box	block containers and inline boxes	no	CSS Inline 3
text-box-trim	block containers and inline boxes	no	CSS Inline 3
text-box-edge	block containers and inline boxes	no	CSS Inline 3
text-group-align	block containers	no	CSS Text 4
text-indent	block containers	yes	CSS Text 3, CSS Text 4, CSS 2.1
text-overflow	block containers	no	CSS Overflow 3, CSS Overflow 4, CSS UI 3
text-wrap-style	block containers hat establish an inline formatting context	yes	CSS Text 4
widows	block containers that establish an inline formatting context	yes	CSS Break 3, CSS Break 4
white-space-trim	inline boxes and block containers	no	CSS Text 4

Limiting this list to those that only apply to block containers and are not inherited, we get:

• text-group-align
• text-overflow

And text-group-align isn't stable yet, and according to a spec. issue might turn into an inherited property. So we're left with text-overflow.

For the non-inherited properties that apply to more than just block containers and don't have appropriate checks or bug reports, I'll create them.

Julian, one question: Should also not yet implemented features be added? My opinion is, yes, but only those that are somewhat stable regarding the spec. or are already implemented in other browers.

Sebastian

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for going through the spec in details Sebastian.

Regarding not implemented features, I am not sure what we should do. My main concern is that if Firefox doesn't implement the related feature yet, developers won't be able to see the impact when fixing their CSS code and this could make the warning hard to understand?

I feel like we should stick to implemented + spec stable features for the warnings, but we'll discuss it in the next triage session.

Comment 4

[Sebastian Zartner [:sebo]]

Happy New Year, Julian! And thank you for your reply!

(In reply to Julian Descottes [:jdescottes] from comment #3)

Regarding not implemented features, I am not sure what we should do. My main concern is that if Firefox doesn't implement the related feature yet, developers won't be able to see the impact when fixing their CSS code and this could make the warning hard to understand?

Unimplemented features are struck through and have a warning icon, anyway. So developers won't see the inactive CSS warning until Firefox implements the feature.

I feel like we should stick to implemented + spec stable features for the warnings, but we'll discuss it in the next triage session.

Please let me know of the outcome of that discussion!

Sebastian

Comment 5

[Sebastian Zartner [:sebo]]

I just realized that there are many special cases to consider regarding block containers, initially mentioned by Daniel in https://bugzilla.mozilla.org/show_bug.cgi?id=1551578#c12. And there was a discussion between Mats and Emilio about whether to expose an API in the InspectorUtils in https://phabricator.services.mozilla.com/D62407. It looks like that API got implemented in form of getContainingBlock() in bug 1601083. Though I'll need to try out whether those special cases can be covered using that API.

Sebastian

Comment 6

[Sebastian Zartner [:sebo]]

Nope, doesn't work. Daniel, Emilio, is there some other API that can be used to identify whether an element is a block container or has this to be implemented in JS? Considering all the edge cases is quite hard and according to Mats there's no "100% correct solution in JS unless we expose a lot more details of the box tree".

Sebastian

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Sebastian Zartner [:sebo] from comment #4)

Happy New Year, Julian! And thank you for your reply!

(In reply to Julian Descottes [:jdescottes] from comment #3)

Regarding not implemented features, I am not sure what we should do. My main concern is that if Firefox doesn't implement the related feature yet, developers won't be able to see the impact when fixing their CSS code and this could make the warning hard to understand?

Unimplemented features are struck through and have a warning icon, anyway. So developers won't see the inactive CSS warning until Firefox implements the feature.

Oh good point, that would be fine to implement the warning then, as long as the spec is clear on that topic. Might just be a bit hard to test, but we could have follow up bugs for this.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

I'm confused, getContainingBlock() isn't related to whether the thing is a block container at all. That just gives you the actual containing block.

There's no api to get whether you're a block container but it's trivial to write. The relevant C++ code could be something like:

nsIFrame* f = aElement.GetPrimaryFrame(FlushType::Frames);
if (!f) { return false; }
if (f->IsBlockFrameOrSubClass()) { return true; }
if (nsIFrame* inner = f->GetContentInsertionFrame()) {
  return inner->IsBlockFrameOrSubClass();
}
return false;

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

There's no api to get whether you're a block container but it's trivial to write. The relevant C++ code could be something like:

nsIFrame* f = aElement.GetPrimaryFrame(FlushType::Frames);
if (!f) { return false; }
if (f->IsBlockFrameOrSubClass()) { return true; }
if (nsIFrame* inner = f->GetContentInsertionFrame()) {
  return inner->IsBlockFrameOrSubClass();
}
return false;

+1 to this. I think this should cover the scenarios that I was thinking about in bug 1551578 comment 12, to basically answer "does this element generate a nsBlockFrame that block-specific styles would conceivably apply to".

Comment 10

[Julian Descottes [:jdescottes]]

Please let me know of the outcome of that discussion!

Discussed in triage, sounds fine to add the warnings even if Firefox doesn't support the features yet.
For tests we might have to update the harness (or rather the test helpers we use for inactive CSS) so that we can expect some tests to fail until the feature is there.

Comment 11

[Sebastian Zartner [:sebo]]

Attachment: Bug 1583902 - [devtools] Added InspectorUtils.isBlockContainer to check whether an element is a block container. r=emilio,dholbert

Comment 12

[Sebastian Zartner [:sebo]]

Attachment: Bug 1583902 - [devtools] Handle ignored properties in non-block containers in inactive CSS. r=#devtools-reviewers,jdescottes

The claim that text-overflow is always active when overflow:hidden is set actually does not seem to be correct.
In particular, for non-block containers, the text-overflow property has no effect even if overflow:hidden is set.

Depends on D261339

Comment 13

[Sebastian Zartner [:sebo]]

Emilio, as discussed, maybe you could have a look at the display: block ruby issue. To me it looks like we have to check the wrapper frame in case it's a ruby frame. So https://searchfox.org/firefox-main/rev/60308bc3792ef201b82377682de068a5a1c72575/layout/base/nsCSSFrameConstructor.cpp#3265 seems to be related.

Sebastian

Comment 14

[Emilio Cobos Álvarez [:emilio]]

No, the issue is that in this case we have:

ScrollContainer
  Block
    Ruby

ScrollContainerFrame::GetContentInsertionFrame() will jump across the block and return the ruby (because it calls GetContentInsertionFrame on its own).

A simple fix could be something like:

diff --git a/layout/inspector/InspectorUtils.cpp b/layout/inspector/InspectorUtils.cpp
index 16d84c9372e6..4a1dc955f614 100644
--- a/layout/inspector/InspectorUtils.cpp
+++ b/layout/inspector/InspectorUtils.cpp

   if (!frame) {
     return false;
   }
-
   // For fieldset elements, we need to check the inner frame.
-  nsFieldSetFrame* fieldsetFrame = do_QueryFrame(frame);
-  if (fieldsetFrame) {
+  if (nsFieldSetFrame* fieldsetFrame = do_QueryFrame(frame)) {
     frame = fieldsetFrame->GetInner();
   }
-
   if (frame->IsBlockFrameOrSubclass()) {
     return true;
   }
+  if (auto* sc = frame->GetScrollTargetFrame()) {
+    if (sc->GetScrolledFrame()->IsBlockFrameOrSubclass()) {
+      return true;
+    }
+  }
   if (nsIFrame* inner = frame->GetContentInsertionFrame()) {
-    return inner->IsBlockFrameOrSubclass();
+    if (inner->IsBlockFrameOrSubclass()) {
+      return true;
+    }
   }
-
   return false;
 }

Comment 15

[Sebastian Zartner [:sebo]]

Muchas gracias, Emilio! Now it works like a charm.

Sebastian

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/aeff976d1eab
https://hg.mozilla.org/integration/autoland/rev/3fa1d43d25a4
[devtools] Added InspectorUtils.isBlockContainer to check whether an element is a block container. r=emilio,layout-reviewers
https://github.com/mozilla-firefox/firefox/commit/cf6f2a7afb8b
https://hg.mozilla.org/integration/autoland/rev/57af52051279
[devtools] Handle ignored properties in non-block containers in inactive CSS. r=fluent-reviewers,nchevobbe,flod

Comment 17

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3fa1d43d25a4
https://hg.mozilla.org/mozilla-central/rev/57af52051279