Closed Bug 1551578 Opened 3 years ago Closed 3 years ago

[Inactive CSS] Display a warning when text-overflow is used incorrectly

Categories

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

enhancement

Tracking

(firefox75 fixed)

RESOLVED FIXED
Firefox 75
Tracking Status
firefox75 --- fixed

People

(Reporter: miker, Assigned: pranavpandey1998official, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Importance: 61.7%])

Attachments

(1 file)

Only valid if not a block container element or doesn't clip content:

[
  "text-overflow",
]

[This property] doesn't have an effect because the element is not a block level element or overflow:hidden is set. Try adding a property like display:block or overflow:auto.

Hey Mike, I would like to work on this. Can you please guide me about where do I need to make changes ?

Flags: needinfo?(pbrosset)
Flags: needinfo?(mratcliffe)

Great, thanks for your interest.
I've assigned the bug to you.
I'm also marking Mike as the mentor on this bug as he is the best person to help you on this. But let me try to get you on the right path:

The code that needs to be changed is in this file /devtools/server/actors/utils/inactive-property-helper.js.
This contains a sort of rules engine that, given a dom node, a css rule, and a css property name decides whether that property is inactive or not.
The file already contains a number of rules for flex and grid specific css properties (see the VALIDATORS function).
You will need to add a new one specifically for the text-overflow property, so a warning is emitted when it is used on a node that this property does not apply to.

If you look around in this file, you will see a number of helper functions to help you do this. You can check what styles are currently applied to the node for instance with checkStyle.

Now, the second part is the warning itself. It needs to be localized, and therefore is referenced in a localization file: /devtools/client/locales/en-US/tooltips.ftl (this is using the fluent library: https://wiki.mozilla.org/Fluent).

I suggest looking at how other rules do it and doing something similar.

Assignee: nobody → dhyey35
Mentor: mratcliffe
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Flags: needinfo?(mratcliffe)

(In reply to Dhyey Thakore [:dhyey35] from comment #1)

Hey Mike, I would like to work on this. Can you please guide me about where do I need to make changes?

Please do as Patrick suggested and if you have any issues please needinfo me here again and I will be more than happy to help.

Hi Mike, how can I determine if a content is clipped or not ?

Flags: needinfo?(mratcliffe)

You just need to check whether the element is a block level element or whether overflow:hidden is set.

Content can be clipped if overflow:hidden is set.

Flags: needinfo?(mratcliffe)
Priority: P2 → P3

Hi Dhyey. I'm wondering if you are still planning on addressing this bug or not. If you just need more time that's fine. No rush.
If you however don't plan on fixing it anymore, please let me know.

Flags: needinfo?(dhyey35)

Unassigning this bug for now. Feel free to comment if you are interested in fixing this bug.

Assignee: dhyey35 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dhyey35)
Whiteboard: [Importance: 61.7%]

I want to work on this issue

Hi Pranav, thank you for your interest. This bug is available for you to work on, so I'm assigning it to you now.

You will see a lot of information about how to start working on this in comment 2 of this bug.
But before that, since you seem to be new here, please take a look at our contributor guide in order to learn about how to get the code, build and make changes to DevTools: https://docs.firefox-dev.tools/

Assignee: nobody → pranavpandey1998official
Status: NEW → ASSIGNED

Hey Daniel, I'd like to double check one thing with you here before landing this new warning in DevTools. Our plan is to display a warning telling users that text-overflow has no effect on an element if that element is not a block container (display is not any of block or inline-block), and if that element does not have overflow set to hidden.

Is that going to work in all cases, or are we likely to run into false positives (where a warning is shown despite text-overflow actually having effect)? I'm thinking that there might be special cases with grid or flex or tables perhaps.

Flags: needinfo?(dholbert)

As long as you're testing computed style when looking for block/inline-block, then I think that mostly makes sense. (This is needed in order to catch the automatic promotion of display:inline elements to display:block when they are floated/abspos/flex-or-grid-items -- e.g. <span style="float:left">)

However, there are some exceptions. The first one is related to tables, as you suspected -- you do need to check for display:table-cell (along with block and inline), because table cells have an anonymous block that wraps their contents, which inherits a bunch of properties, including text-overflow. Here's an example where this property has an effect, with a table-cell:

data:text/html,<span style="display:table-cell;max-width:50px;overflow:hidden;text-overflow:ellipsis">abcdefghijklmnop

This comes into play via this ua.css style rule, BTW (which is where the style inherits from the cell to its anonymous block):
https://searchfox.org/mozilla-central/rev/7e92a667e3829831c31e8d46aefe7ef67ad5be1c/layout/style/res/ua.css#160

(Grid & flex anonymous wrappers do not inherit text-overflow so this isn't an issue for grid/flex AFAICT).

I don't think any of the other text-overflow: inherit declarations in that file (ua.css) are places that would be problematic with your described logic, which is good news. (I wonder a bit about the -moz-block-inside-inline-wrapper one, but I played around & couldn't come up with a way to make text-overflow matter on a display:inline element involved in a block-in-inline split.)

But you do also need to check form controls, many of which also have anonymous block wrappers inside of them, even if they're display:inline (and they inherit text-overflow and overflow into those wrappers). I'm thinking fieldset and button in particular, but really all of the elements listed in forms.css above this style rule with text-overflow:inherit:
https://searchfox.org/mozilla-central/rev/7e92a667e3829831c31e8d46aefe7ef67ad5be1c/layout/style/res/forms.css#769

Some examples that I tested where text-overflow matters for form elements (even if they're display:inline) (note that I'm manually specifying display:inline here, just for clarity/good-measure, even though in some cases it's already the default):

data:text/html,<button style="display:inline;max-width:100px;overflow:hidden;text-overflow:ellipsis">abcdefghijklmnopqrstuv

data:text/html,<fieldset style="display:inline;min-width:0;max-width:50px;overflow:hidden;text-overflow:ellipsis">abcdefghijklmnopqrstuv

data:text/html,<input style="display:inline;min-width:0;max-width:100px;overflow:hidden;text-overflow:ellipsis" value="abcdefghijklmnop">

data:text/html,<select style="display:inline;max-width:100px;overflow:hidden;text-overflow:ellipsis"><option>abcdefghijklmnopqrstuv

As an extra sanity-check, I'll punt your needinfo to mats, since he worked on text-overflow:ellipsis and he may know if there's any other exceptions (besides these ones) that your described logic wouldn't cover.

Flags: needinfo?(dholbert) → needinfo?(mats)

Also the fieldset <legend> child:

data:text/html,<fieldset><legend style="display:inline;max-width:50px;overflow:hidden;text-overflow:ellipsis">abcdefghijklmnopqrstuv

(commented inline in the patch)

Flags: needinfo?(mats)
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4222d56ec9d
"Bug 1551578 - displayed a warning when text-overflow is used incorrectly.  " r=pbro
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75
You need to log in before you can comment on or make changes to this bug.