Skip to content

Conversation

@francescorn
Copy link
Contributor

@francescorn francescorn commented Dec 9, 2023

793932c

Web Inspector: Font Panel: CSS font property values marked !important don't get overridden when using the interactive editing controls.
https://bugs.webkit.org/show_bug.cgi?id=258975

Reviewed by Devin Rousso.

If a CSS class has a !important tag on a property the slider does not work correctly.
This is due to us modifying the inline style, and the inline style is being overriden by
the css class !important property.

We can address this by checking if the !important property is being applied already
during _computeProperty(), and then if it is then append !important in the
writeFontVariation().

Rename resultProperties to resultProperty inside of _computeProperty() because there
is a single property being handled.

Delete _effectivePropertyValueForName function since it is not in use and create it in
_computeProperty instead to be used for resultProperty.isImportant.

* Source/WebInspectorUI/UserInterface/Models/FontStyles.js:
(WI.FontStyles.prototype.writeFontVariation):
(WI.FontStyles):

Canonical link: https://commits.webkit.org/275389@main

ff503c3

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
loading 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@francescorn francescorn self-assigned this Dec 9, 2023
@francescorn francescorn added the Web Inspector Bugs related to the WebKit Web Inspector. label Dec 9, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 9, 2023
@francescorn francescorn removed the merging-blocked Applied to prevent a change from being merged label Dec 11, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 11, 2023
@francescorn francescorn marked this pull request as draft December 12, 2023 18:12
@rcaliman-apple
Copy link
Contributor

I see this is still a draft so I won't review it yet.

I also notice you closed the previous PR for the same task: #20015 That's unfortunate. There are useful comments in there. In general, it's preferable to keep the same PR and iterate on the code to maintain that history for people in the future who research why a patch was made a certain way.

Copy link
Contributor

@rcaliman-apple rcaliman-apple Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to modify the value here in the parsing stage when receiving it from the backend.
This is causing the inspector/css/modify-inline-style.html test to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it

@francescorn francescorn removed the merging-blocked Applied to prevent a change from being merged label Dec 15, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 16, 2023
@francescorn francescorn marked this pull request as ready for review December 16, 2023 03:20
@francescorn francescorn marked this pull request as draft January 31, 2024 00:50
@francescorn francescorn removed the merging-blocked Applied to prevent a change from being merged label Feb 21, 2024
Comment on lines 142 to 143
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change always add the !important suffix to the written property value.
It should be added only when the property value collected from the page originally was marked with !important.

To find identify that, you can update WI.FontStyles._computeProperty() to look at the important property of the WI.CSSProperty collected into WI.DOMNodeStyles.

Like this:

// returns true if the CSS property was authored with `!important` in its value (which exists on WI.CSSProperty._rawValue)
style.domNodeStyle.effectivePropertyForName(name).important

Update the object returned by WI.FontStyles._computeProperty with an additional property, like isImportant or similar.

Note

The name of that return object is misleadingly pluralised now as resultProperties. It should be resultProperty since that method operates on a single property. Probably a typo from when it was written.

If you follow that return object, you'll notice it gets collected with the other ones for other CSS properties in WI.FontStyles._propertiesMap.

That's the map you can then check against in WI.FontStyles.writeFontVariation() to check whether the value of the CSS property you're trying to update was originally marked important. If so, concatenate the "!important" to targetPropertyValue.

You don't have to make any changes outside of WI.FontStyles to fix this bug. Please don't change WI.CSSProperty

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inspectorOverrideImportant does not exist on WI.CSSProperty. Please remove.

@francescorn
Copy link
Contributor Author

themes.tiki.org works now, I appended !important whenever there is a CSS property with !important.
!important_bugfix_themes.tiki.org

@francescorn francescorn marked this pull request as ready for review February 23, 2024 00:38
Copy link
Contributor

@rcaliman-apple rcaliman-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this.

Copy link
Contributor

@rcaliman-apple rcaliman-apple Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code comment is redundant. Please remove it. I had put it in the code snippet in my review as an explanation for you

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename resultProperties to resultProperty inside this method. The plural form is a typo from the original patch. This method deals with a single property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we're already calling this inside _effectivePropertyValueForName, so i think we could/should avoid that

let effectivePropertyForName = style.domNodeStyle.effectivePropertyForName(name);

let value = effectivePropertyForName?.value;

...

resultProperty.isImportant = effectivePropertyForName?.important ?? false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Copy link
Contributor Author

@francescorn francescorn Feb 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep return resultProperties included

Copy link
Contributor Author

@francescorn francescorn Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing return resultProperties was causing web inspector test cases to fail, so I added it back

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
@francescorn francescorn removed the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
@francescorn francescorn requested a review from dcrousso February 23, 2024 05:47
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
@francescorn francescorn removed the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@francescorn francescorn removed the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@francescorn francescorn removed the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@rcaliman-apple
Copy link
Contributor

I'm ok with this landing. @dcrousso want to approve?

@stwrt stwrt added the merge-queue Applied to send a pull request to merge-queue label Feb 27, 2024
… don't get overridden when using the interactive editing controls.

https://bugs.webkit.org/show_bug.cgi?id=258975

Reviewed by Devin Rousso.

If a CSS class has a !important tag on a property the slider does not work correctly.
This is due to us modifying the inline style, and the inline style is being overriden by
the css class !important property.

We can address this by checking if the !important property is being applied already
during _computeProperty(), and then if it is then append !important in the
writeFontVariation().

Rename resultProperties to resultProperty inside of _computeProperty() because there
is a single property being handled.

Delete _effectivePropertyValueForName function since it is not in use and create it in
_computeProperty instead to be used for resultProperty.isImportant.

* Source/WebInspectorUI/UserInterface/Models/FontStyles.js:
(WI.FontStyles.prototype.writeFontVariation):
(WI.FontStyles):

Canonical link: https://commits.webkit.org/275389@main
@webkit-commit-queue
Copy link
Collaborator

Committed 275389@main (793932c): https://commits.webkit.org/275389@main

Reviewed commits have been landed. Closing PR #21557 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 27, 2024
@webkit-commit-queue webkit-commit-queue merged commit 793932c into WebKit:main Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Web Inspector Bugs related to the WebKit Web Inspector.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants