[TASK] Remove the IE hack in `Rule` #995

oliverklee · 2025-02-25T10:51:06Z

No description provided.

coveralls · 2025-02-25T10:52:46Z

coverage: 55.987% (-0.02%) from 56.006%
when pulling b0c8d8f on task/remove-ie-hack
into 54ca442 on main.

JakeQZ

#993 deprecated this functionality in the 8,x branch, so we can remove it for 9.0 (provided we do actually release 8.8 first). However, one deprecation was missing from that change. (And a private property was unnecessarily deprecated.)

Also, if we remove support for IE hacks, we should not then allow CSS rules containing them to be parsed as valid.

JakeQZ · 2025-02-25T23:25:00Z

src/Rule/Rule.php

        if ($parserState->getSettings()->bLenientParsing) {
            while ($parserState->comes('\\')) {
                $parserState->consume('\\');
-                $rule->addIeHack($parserState->consume());
+                $parserState->consume();
                $parserState->consumeWhiteSpace();
            }
        }


I think this whole code block is parsing the IE hack(s). And the idea is that IE will treat them as valid rules whilst other browsers will discard them. There's a mention in the readme that IE hacks are only supported in lenient mode. So, on that basis, I think the entire if statement should be removed, so that invalid declarations with IE hacks are discarded rather than kept.

src/Rule/Rule.php

JakeQZ

This still isn't quite right, but I'm not sure why. The rules with IE hacks should be dropped entirely (in lenient mode when an exception is not thrown).

If only the IE hack part of the value is dropped, the result is a property declaration that all browsers would apply. Whereas with the IE hack, it is only applied in IE.

oliverklee · 2025-02-26T10:19:18Z

The rules with IE hacks should be dropped entirely (in lenient mode when an exception is not thrown).

The removed code was the only IE-hack-specific code I could find. My best guess ist that the rest is from "the way lenient parsing currently works".

I'd like to keep this PR focused on removing these specific pieces of code, and keep "cleaning up lenient parsing" out of it. I'm aiming for "remove what I can find about our special cases for IE hacks without breaking anything", not for "add a feature to clean up and remove IE hacks in the input CSS".

@JakeQZ Would this be okay for you?

oliverklee · 2025-02-26T17:14:21Z

I have changed the name of the test from ieHacksAreRemovedInLenientMode to ieHacksArePartiallyRemovedInLenientMode so that it's not misleading anymore.

JakeQZ · 2025-02-26T19:27:26Z

My best guess ist that the rest is from "the way lenient parsing currently works".

I think you're right. Rule::parse() will return a valid Rule whatever junk comes after it (IE hacks or anything else). It does not check that there is a semicolon or closing brace following (and will actually, it seems, parse multiple rules without semicolons between them). These I think are bugs. The exception in strict mode occurs on attempting to parse the 'next' rule, which is actually the junk or IE hack at the end of the previous. So I think this is fine.

I have changed the name of the test from ieHacksAreRemovedInLenientMode to ieHacksArePartiallyRemovedInLenientMode so that it's not misleading anymore.

I'm not sure about this rename - the IE hacks seem to be all removed - things like content: "red \\0" are actual strings, not IE hacks.

JakeQZ

The rules with IE hacks should be dropped entirely (in lenient mode when an exception is not thrown).

The removed code was the only IE-hack-specific code I could find. My best guess ist that the rest is from "the way lenient parsing currently works".

I managed to unearth the change that introduced the IE hacks: e1ecb78

There are two additional changes I can see that also need to be undone.

Value::parseValue() has the responsibility of continually parsing until it finds something that terminates the value, like ;, } or ! (for !important). There it is also testing for \ for the IE hacks. So that conditional should be removed. Then we might find the rules with IE hacks are dropped as they should be (and would be by non-IE browsers).
- If the above so far is good, the test method in lenient mode can be renamed to indicate that rules with IE hacks will be discarded.
ParserState::parseCharacter has special-case handling of \0 and \9 which should also be removed.

I am not sure why Value::parseValue() is also stopping at ). I have a vague recollection of that also being used in an IE hack. However, the Rule class doesn't appear to support that as an IE hack (if it ever did), and that was already in place before the IE hack support was added. That's beyond the scope of this PR.

oliverklee · 2025-02-27T09:06:39Z

I have removed the other IE-hacks-related code parts now. I've also removed the tests as we now don't have a special case anymore (and I failed to create a usable expected string to compare with).

JakeQZ · 2025-02-27T10:42:39Z

I have removed the other IE-hacks-related code parts now. I've also removed the tests as we now don't have a special case anymore (and I failed to create a usable expected string to compare with).

Could you not just run the tests and capture the (unexpected) output, then paste that in as the expected output?

Having the tests in place was a massive help in working out what wasn't quite working, since you had changed the expected result to match the result after the code change. Having that there saved any actual debugging, as it made it possible to work out what was going on by merely inspecting the code and looking at the output (in the tests). So, for this PR, I would not like the tests removed, because I would like to see the result of the changes in the tests to confirm it is A-OK.

The tests can be dropped in a subsequent PR.

Does that work for you?

oliverklee · 2025-02-27T11:16:08Z

Could you not just run the tests and capture the (unexpected) output, then paste that in as the expected output?

I tried … and failed to copy'n'paste the actual output as it contained at least a tab and a non-displayable character. Would you be willing to give it a go (after I've rebase this PR and resolved the merge conflicts)?

JakeQZ · 2025-02-27T11:48:59Z

Could you not just run the tests and capture the (unexpected) output, then paste that in as the expected output?

I tried … and failed to copy'n'paste the actual output as it contained at least a tab and a non-displayable character. Would you be willing to give it a go (after I've rebase this PR and resolved the merge conflicts)?

I'll give it a try later. But I am using Windows :/

oliverklee · 2025-02-27T11:54:13Z

I'll give it a try later. But I am using Windows :/

If you get stuck, please let me know. Then I'll try again.

JakeQZ · 2025-02-28T19:13:32Z

I've got a bit further with this. With IE hacks support removed, \9 and \0 are now parsed as escaped characters. So they become TAB and NUL, respectively. Though there seem to be some stray backslashes in the rendered output. I'll come back to this and see if I can work out why that is happening; probably just put the tests back in with the expected output as it will become (so we can still see what's happening), and create a separate issue for the handling of escaped characters outside of strings (although IIRC, strings in CSS don't need quotes), so we can get this PR completed...

JakeQZ

I've added #1053 to capture the issues found.

We can now remove those tests.

JakeQZ · 2025-03-02T23:41:21Z

(although IIRC, strings in CSS don't need quotes)

Actually they do as direct values, though not in arguments to functions like url().

oliverklee added this to the 9.0.0: Drop support for PHP < 7.2 and clean up things milestone Feb 25, 2025

oliverklee requested a review from JakeQZ February 25, 2025 10:51

oliverklee self-assigned this Feb 25, 2025

JakeQZ requested changes Feb 25, 2025

View reviewed changes

oliverklee force-pushed the task/remove-ie-hack branch 2 times, most recently from 1422ea1 to 7448857 Compare February 26, 2025 08:19

oliverklee requested a review from JakeQZ February 26, 2025 08:20

JakeQZ requested changes Feb 26, 2025

View reviewed changes

oliverklee force-pushed the task/remove-ie-hack branch from 7448857 to 706bf32 Compare February 26, 2025 17:13

oliverklee force-pushed the task/remove-ie-hack branch from 706bf32 to 1d0a737 Compare February 26, 2025 18:39

JakeQZ requested changes Feb 27, 2025

View reviewed changes

JakeQZ mentioned this pull request Feb 27, 2025

Value/rule parsing issues #1010

Open

oliverklee force-pushed the task/remove-ie-hack branch from 1d0a737 to a41aff5 Compare February 27, 2025 09:05

oliverklee requested a review from JakeQZ February 27, 2025 09:07

oliverklee force-pushed the task/remove-ie-hack branch from a41aff5 to f2ff2d2 Compare February 27, 2025 09:36

oliverklee force-pushed the task/remove-ie-hack branch 2 times, most recently from 6b78510 to b5dc33b Compare February 27, 2025 11:18

oliverklee force-pushed the task/remove-ie-hack branch 4 times, most recently from 4e7ba3b to 0e1570a Compare February 28, 2025 08:06

oliverklee force-pushed the task/remove-ie-hack branch from 0e1570a to c778cfa Compare March 2, 2025 14:07

oliverklee added 3 commits March 2, 2025 20:33

[TASK] Remove the IE hack in Rule

3b9b115

Changes suggested in code review

1ef8244

Remove more ancient stuff

b0c8d8f

oliverklee force-pushed the task/remove-ie-hack branch from c778cfa to b0c8d8f Compare March 2, 2025 19:33

JakeQZ mentioned this pull request Mar 2, 2025

Issues with parseIdentifier and escaped control characters #1053

Open

JakeQZ approved these changes Mar 2, 2025

View reviewed changes

JakeQZ merged commit 08010a2 into main Mar 2, 2025
21 checks passed

JakeQZ deleted the task/remove-ie-hack branch March 2, 2025 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TASK] Remove the IE hack in `Rule` #995

[TASK] Remove the IE hack in `Rule` #995

oliverklee commented Feb 25, 2025

coveralls commented Feb 25, 2025 •

edited

Loading

JakeQZ left a comment

JakeQZ Feb 25, 2025

JakeQZ left a comment

oliverklee commented Feb 26, 2025 •

edited

Loading

oliverklee commented Feb 26, 2025

JakeQZ commented Feb 26, 2025

JakeQZ left a comment

oliverklee commented Feb 27, 2025

JakeQZ commented Feb 27, 2025

oliverklee commented Feb 27, 2025

JakeQZ commented Feb 27, 2025

oliverklee commented Feb 27, 2025

JakeQZ commented Feb 28, 2025

JakeQZ left a comment

JakeQZ commented Mar 2, 2025 •

edited

Loading

[TASK] Remove the IE hack in Rule #995

[TASK] Remove the IE hack in Rule #995

Conversation

oliverklee commented Feb 25, 2025

coveralls commented Feb 25, 2025 • edited Loading

JakeQZ left a comment

Choose a reason for hiding this comment

JakeQZ Feb 25, 2025

Choose a reason for hiding this comment

JakeQZ left a comment

Choose a reason for hiding this comment

oliverklee commented Feb 26, 2025 • edited Loading

oliverklee commented Feb 26, 2025

JakeQZ commented Feb 26, 2025

JakeQZ left a comment

Choose a reason for hiding this comment

oliverklee commented Feb 27, 2025

JakeQZ commented Feb 27, 2025

oliverklee commented Feb 27, 2025

JakeQZ commented Feb 27, 2025

oliverklee commented Feb 27, 2025

JakeQZ commented Feb 28, 2025

JakeQZ left a comment

Choose a reason for hiding this comment

JakeQZ commented Mar 2, 2025 • edited Loading

[TASK] Remove the IE hack in `Rule` #995

[TASK] Remove the IE hack in `Rule` #995

coveralls commented Feb 25, 2025 •

edited

Loading

oliverklee commented Feb 26, 2025 •

edited

Loading

JakeQZ commented Mar 2, 2025 •

edited

Loading