Bug 256754

Summary:	Align with 'UA' Stylesheet for 'lists' by changing '1__qem' to '1em' for `margin-block-start`
Product:	WebKit	Reporter:	Ahmad Saleem <ahmad.saleem792>
Component:	CSS	Assignee:	Nobody <webkit-unassigned>
Status:	NEW ---
Severity:	Normal	CC:	karlcow, ntim, simon.fraser, webkit-bug-importer, zalan
Priority:	P2	Keywords:	BrowserCompat, InRadar
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified
See Also:	https://bugs.webkit.org/show_bug.cgi?id=253177 https://bugs.webkit.org/show_bug.cgi?id=256760

Description Ahmad Saleem 2023-05-13 01:34:01 PDT

Hi Team,

While we are going to sync-up partial UA rules, based on Tim's suggestion, I am tackling this case separately since it would require further discussion.

HTML Spec: https://html.spec.whatwg.org/#lists

Current use of '1__qem':

ol {
...
margin-block-start: 1__qem;
...
}

and

dl {
...
margin-block-start: 1__qem;
...
}

__________________

As per Web-Spec and aligning with 'Firefox', we have to use '1em'.

This was added by this commit in 2002 - https://github.com/WebKit/WebKit/commit/ad72cdff4371a3c296ab7a7e162620ebdc940325

__________________

Just wanted to raise this.

Thanks!

Comment 1 Ahmad Saleem 2023-05-13 01:39:23 PDT

Also:

ul, menu, dir {
...
margin-block-start: 1__qem;
...
}

Comment 2 Ahmad Saleem 2023-05-13 11:43:02 PDT

(In reply to Ahmad Saleem from comment #1)
> Also:
> 
> ul, menu, dir {
> ...
> margin-block-start: 1__qem;
> ...
> }

Also this bit:

ul, menu {
    counter-reset: list-item;
}

Comment 3 Radar WebKit Bug Importer 2023-05-20 01:34:17 PDT

<rdar://problem/109600780>

Comment 4 Karl Dubost 2023-05-21 21:30:44 PDT

tldr:
* safe to remove from html.css
* Quirk Unit code could be removed in another bug(?) Let's ask Alan?


Details:

Before making a decision on that, it would be good to check in the code.
I see in the code some specific usage for qem

* CSS_QUIRKY_EMS
  https://searchfox.org/wubkat/search?q=CSS_QUIRKY_EMS&path=&case=false&regexp=false
* isQuirkyEms()
  https://searchfox.org/wubkat/search?q=isquirkyems&path=&case=false&regexp=false

isQuirkyEms is not used at all in the code, apart its declaration. Probably can be removed. 

https://searchfox.org/wubkat/rev/622bd3bfe173b244d3cc68f2cb14ebbf987e14dd/Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp#542-545

        case CSSUnitType::CSS_QUIRKY_EMS:
            if (parserMode != UASheetMode)
                return std::nullopt;
            FALLTHROUGH;
 
and other cases with no difference with the standard ems. 

but I'm not even sure it has the impact it had in the past.
https://searchfox.org/wubkat/rev/622bd3bfe173b244d3cc68f2cb14ebbf987e14dd/Source/WebCore/css/CSSUnits.h#121-125

    // This value is used to handle quirky margins in reflow roots (body, td, and th) like WinIE.
    // The basic idea is that a stylesheet can use the value __qem (for quirky em) instead of em.
    // When the quirky value is used, if you're in quirks mode, the margin will collapse away
    // inside a table cell. This quirk is specified in the HTML spec but our impl is different.
    CSS_QUIRKY_EMS

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

Is the implementation handling it now directly?
https://searchfox.org/wubkat/rev/622bd3bfe173b244d3cc68f2cb14ebbf987e14dd/Source/WebCore/layout/formattingContexts/table/TableFormattingGeometry.cpp#46-62

when this was written?
https://searchfox.org/wubkat/commit/ab3b5a332211ad9e67239fe06dce4ec9f5d155b7

I wonder if all of this can be retired



Probably it would be safe to remove from the css. 
https://searchfox.org/wubkat/search?q=_qem&path=&case=false&regexp=false

And there is a need for a followup bug on removing the QEM code.

Comment 5 Karl Dubost 2023-05-22 15:09:37 PDT

Ah wait, I'm walking back my previous comment. 

I had missed this part 
https://searchfox.org/wubkat/rev/622bd3bfe173b244d3cc68f2cb14ebbf987e14dd/Source/WebCore/style/StyleBuilderConverter.h#233-237

which associates the qem to the quirk behavior.


    if (primitiveValue.isLength()) {
        Length length = primitiveValue.computeLength<Length>(conversionData);
        length.setHasQuirk(primitiveValue.primitiveType() == CSSUnitType::CSS_QUIRKY_EMS);
        return length;
    }

ok we should make a test and change the CSS to see what differences it creates for the tables.