Closed Bug 1551581 Opened 5 years ago Closed 5 years ago

[Inactive CSS] Display a warning when `display:*` is used on a floated element

Categories

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

Product:
DevTools
Component:
Inspector: Rules
Type:
enhancement

Tracking

(firefox71 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: miker, Assigned: biboswan98)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Importance: 92.3%])

Attachments

(1 file, 1 obsolete file)

Bug 1551581 - New InactiveCSS rule to show warning when display:* used on a floated element; r=miker, pbro
47 bytes, text/x-phabricator-request
Details | Review

Only non-floated elements:

[
  "display",
]

[This property] doesn't have an effect because it cannot be used on floated elements.

This MDN page is quite useful for display used on floated elements: https://developer.mozilla.org/en-US/docs/Web/CSS/float
In particular, it mentions which display values are replaced. Here they are:

  • inline --> block
  • inline-block --> block
  • inline-table --> table
  • table-row --> block
  • table-row-group --> block
  • table-column --> block
  • table-column-group --> block
  • table-cell --> block
  • table-caption --> block
  • table-header-group --> block
  • table-footer-group --> block
  • inline-flex --> flex
  • inline-grid --> grid

So, it might be nice top make this rule user-friendly here by doing this:

  • if the display value is not one from this list, no warning
  • if it is, show a warning like The display value has been changed by the engine to block because the element is floated.

Hi, I would like to work on this. I see that the computed pane of the devtools shows that the display is block when the display is declared as something like inline on a floated element.
Just wanted to be doubly sure that declaration override is only shown for a declaration which is overridden due to another user-defined css declaration. Engine changed styling like the one mentioned here is not depicted by an override.

We have assigned you to the bug... if you have any questions then please don't hesitate to ask.

Assignee: nobody → biboswan98
Status: NEW → ASSIGNED
Priority: P2 → P3

Hi Biboswan, how are things going? Are you still interested in this bug?
If not, just let me know since other people might be interested in fixing it too! Thanks.

Flags: needinfo?(biboswan98)

Hey Patrick, I will send a PR in 1 or 2 days

Flags: needinfo?(biboswan98)

I wanted to console.log the rules of the inspector. So I edited a rule declaration in devtools and expected to get logs i have put in the action files, components/RulesApp etc in the browser toolbox console. I think some of my assumption is wrong ?
I can easily see corresponding logs or changes i did in the index.xhtml

(In reply to Biboswan Roy from comment #6)

I wanted to console.log the rules of the inspector. So I edited a rule declaration in devtools and expected to get logs i have put in the action files, components/RulesApp etc in the browser toolbox console. I think some of my assumption is wrong ?
I can easily see corresponding logs or changes i did in the index.xhtml

console.log is not always visible in the browser console. Sometimes you need to open the "Browser Content Toolbox" to see the output.

When you ask questions then please also check "Request information from" and choose "reporter" from the dropdown (towards the bottom of this page).

Hi Mike,I wasn't sure whom to ask. I can't see the consoles i put inside reducer and action creator functions in the browser toolbox (Developer Tools- chrome://browser/content/browser.xhtml) Seems like those aren't triggered. So i 'm telling what I did again. I open the nightly build and opened a web page and inspector tools and edited rules of that page. So I expected to see logs due to the UPDATE_RULES action triggered. I also checked the source code in the debugger and my code changes are visible in the source code. But I don't see any such traces. I tried browser content toolbox as well.
Thanks for your patience again

Flags: needinfo?(mratcliffe)

Thanks for the direction Mike. My main concern was not able to see the logs. I will check

Flags: needinfo?(biboswan98)

I 'm implemented the feature. I'm requesting for code review. Who all to request for code reviews as per https://docs.firefox-dev.tools/contributing/making-prs.html what names to include?

Flags: needinfo?(mratcliffe)

I had 2 things to ask.

  1. I haven't added any testing. Do they exist for these kind of bugs
  2. Currently the learn-more on hover links to the inactive property on which the warning is shown. Eg if shown on display, learn-more points to mozilla docs @ display. But for this bug i think display is the inactive property but learn-more shall point to float.

Have a look at my concern in the above comment point2

I see, can you log a bug for that to allow us to override the property that we display the info for?

Flags: needinfo?(mratcliffe) → needinfo?(biboswan98)

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #15)

I see, can you log a bug for that to allow us to override the property that we display the info for?

Yes sure and I would like to implement that myself as well

Flags: needinfo?(biboswan98)

(In reply to Biboswan Roy from comment #8)

Hi Mike,I wasn't sure whom to ask. I can't see the consoles i put inside reducer and action creator functions in the browser toolbox (Developer Tools- chrome://browser/content/browser.xhtml) Seems like those aren't triggered. So i 'm telling what I did again. I open the nightly build and opened a web page and inspector tools and edited rules of that page. So I expected to see logs due to the UPDATE_RULES action triggered. I also checked the source code in the debugger and my code changes are visible in the source code. But I don't see any such traces. I tried browser content toolbox as well.

Hi Biboswan,

In response to this earlier message, the React/Redux files you're trying to use here aren't the ones for the current Rules panel. They're part of a refactoring that's incomplete and not active yet in DevTools. Everything in: new-rules.js, components/, reducers/ and actions/ is part of this refactoring.

The active code handling the existing Rules panel is in rules.js, models/ and views/. This is where you should observe for events and changes.

Sorry about this confusion.

Whiteboard: [Importance: 92.3%]

Thanks Razvan

@biboswan98 Are you still working on this?

Flags: needinfo?(biboswan98)

I will look at it today and try to push by tomorrow

Flags: needinfo?(biboswan98)
Attachment #9094022 - Attachment is obsolete: true

Pushed code again. Have a look

@Biboswan Do you have permission to land this? If not just add the checkin-needed keyword and we will land it for you.

Thanks for working on this.

Flags: needinfo?(biboswan98)

checkin-needed

Flags: needinfo?(biboswan98)
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7be1052a9327
New InactiveCSS rule to show warning when display:* used on a floated element; r=miker,pbro,fluent-reviewers,flod

https://hg.mozilla.org/mozilla-central/rev/7be1052a9327

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

I think inline-flex/grid here need different messages since they are changed to flex/grid as opposed to block.

(In general, the display-outside value will be promoted to block, see https://drafts.csswg.org/css-display/#transformations )

Yes you are right.

So inline-table => table
inline-grid => grid
inline-flex => flex
ruby => block ruby

Then shall there be specific messages for each of these 4 cases. Or provide a generalised message. The engine blockifies the outer display type on a floated element, hence inline-grid is changed to grid, inline-table to table... etc.

Flags: needinfo?(pbrosset)

We should keep it rather generic and then point people to the right MDN documentation so they know more.

Flags: needinfo?(pbrosset)

(In reply to Patrick Brosset <:pbro> from comment #30)

We should keep it rather generic and then point people to the right MDN documentation so they know more.

The current messages aren't really generic though: "The <strong>display</strong> value has been changed by the engine to <strong>block</strong> because the element is <strong>floated<strong>." and "Try removing <strong>float</strong> or adding <strong>display:block</strong>. { learn-more }".

They might be misleading for inline-flex/grid. Should I file a bug about this ?

Flags: needinfo?(pbrosset)

Yes, sorry my answer was a bit short and cryptic. I am in favor of rephrasing the current warning so it is both generic and inclusive of all display types. Feel free to file a bug with a suggestion for a better warning message.

Flags: needinfo?(pbrosset)
Depends on: 1691788
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: