Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UA style for <br> and <wbr> #2291

Open
zcorpan opened this issue Jan 24, 2017 · 22 comments · May be fixed by #2298
Open

UA style for <br> and <wbr> #2291

zcorpan opened this issue Jan 24, 2017 · 22 comments · May be fixed by #2298

Comments

@zcorpan
Copy link
Member

zcorpan commented Jan 24, 2017

See
w3c/csswg-drafts#610 (comment)
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26264#c6
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28080#c3

@domenic
Copy link
Member

domenic commented Jan 24, 2017

This should be a relatively simple spec fix, right?

I guess nobody is going to pass the tests since nobody implements these in CSS from what I understand... not sure what we should do there. Do UAs plan to do so? Maybe we should just give up and stop trying to define these through CSS?

@zcorpan
Copy link
Member Author

zcorpan commented Jan 24, 2017

all: unset is implemented in WebKit/Chromium/Gecko:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4831

display: contents is ... implemented in WebKit/Chromium/Gecko, but with this simple test case there are 3 different behaviors (I think Gecko is per spec):

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4832

I did not test Edge.

Anyway, yeah the spec fix itself should be simple. Slightly more work is probably testing + browser bugs and getting implementors interested in passing the tests...

@domenic
Copy link
Member

domenic commented Jan 24, 2017

In particular the test cases that get the computed (used?) style of br/wbr are the ones I'm worried no engine will ever be interested in passing.

@zcorpan
Copy link
Member Author

zcorpan commented Jan 24, 2017

Hmmm. It's unclear to me if the discussion in https://www.w3.org/Bugs/Public/show_bug.cgi?id=25503 applies equally to the proposed change now. @dbaron?

Also I'll note that content: "foo" doesn't work in most (all?) browsers except for ::before/::after.

@annevk
Copy link
Member

annevk commented Jan 25, 2017

@domenic they pass similar tests for other rules in the user agent stylesheet. Is there a performance concern here? If so, then we should bring that feedback back to the CSS WG, though it would be a little weird for them to recommend something like that then since they have implementers involved too.

@domenic
Copy link
Member

domenic commented Jan 25, 2017

It's more that everyone already has a br implementation that I assume they're happy with. There is a performance concern, which the CSSWG addressed by suggesting the !important reset. That allows implementers to work around the performance issue by having some sort of fast path that uses their existing br logic but reports "fake" used styles. My skepticism is that any implementer would want to implement such a fast path solely for the purpose of passing such tests; it doesn't really have any author or user facing benefit.

Stated another way: it's great that we've shown CSS has a way to express br. But actually mandating that the observable world act as if br was rendered via CSS isn't so useful, I imagine.

Of course this is all speculation. Maybe there were implementers in the room who were already adding this fast path/used style reporting issue to their Q2 goals.

@dbaron
Copy link
Member

dbaron commented Jan 25, 2017

I don't think w3c/csswg-drafts#610 (comment) is an accurate reflection of the WG's discussion (in which I did raise performance concerns), which concluded with:

RESOLVED: keep <br> and <wbr> magic, but add an explainer about how to mimic the results

I was arguing for something similar to what Domenic just wrote as I was typing this, and I think the CSS WG agreed.

@dbaron
Copy link
Member

dbaron commented Jan 25, 2017

Maybe that's not quite accurate, now that I'm remembering the discussion. The idea is that implementations still would have native implementations, but the spec would be defining the expected behavior of those native implementations as equivalent to the given CSS, which (given the all: reset ! important) allows very little styling/customization. The question is if that CSS actually Web-compatibly agrees with what the native <br> implementations do, down to the level of ignoring all CSS properties (including, say, font-size and line-height).

@dbaron
Copy link
Member

dbaron commented Jan 25, 2017

One further note: given that I think the intent is that implementations have non-CSS <br> implementations, I think the spec should (non-normatively) say that that's the intent, so that readers of the spec understand what was intended.

@rniwa
Copy link

rniwa commented Jan 25, 2017

Yeah, it's not really helpful to make these elements be defined in terms of UA stylesheet if that's not what we're trying to achieve. It's rather misleading especially if we haven't figured out the compatibility implications.

domenic added a commit that referenced this issue Jan 25, 2017
domenic added a commit that referenced this issue Jan 25, 2017
@domenic domenic linked a pull request Jan 25, 2017 that will close this issue
@domenic
Copy link
Member

domenic commented Jan 25, 2017

I've posted #2298 as an attempt to concretize this discussion. Given the disappointing lack of interop for the test case at http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4837 , we might not be able to come up with something perfect, but at least what's in that PR seems like a start toward something a bit more aligned with implementations than the fictional display types the spec currently uses.

@tabatkins
Copy link
Contributor

One further note: given that I think the intent is that implementations have non-CSS
implementations, I think the spec should (non-normatively) say that that's the intent, so that readers of the spec understand what was intended.

Yes, that's what the discussion concluded on (implementations should continue to have magic implementations of the elements). @fantasai's post in the issue thread was an inaccurate summary.

@zcorpan
Copy link
Member Author

zcorpan commented Jan 25, 2017

It seems line-height needs to apply to br at least.
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4838

and font-size as well.
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4840

So all: unset !important doesn't match the magic.

@fantasai
Copy link
Contributor

We might want to re-evaluate how revert handles !important origins. If we have it roll back one origin layer only (rather than having UA !important blow out the entire cascade), then we could solve the font-size and line-height aspects by adding font-size: revert !important; line-height: revert !important. Otherwise I'd say, calling out exceptions in prose would be an appropriate approach here. If a UA actually wanted to implement via CSS rules rather than via magic, they'd have to expand out all, but we definitely don't want to do that in the spec.

@tabatkins
Copy link
Contributor

Since we're still not officially defining rendering in terms of CSS, I don't think it's necessary to get the "approximated by this CSS" part precisely right; at least, it's not worth adding complications to other CSS features to make this not-used-in-practice CSS exactly match legacy implementation.

So we can just document the properties that do have an effect in prose.

@domenic
Copy link
Member

domenic commented Mar 14, 2017

Any chance the CSSWG could take an action item for its members to look up what properties apply in their implementations, so we don't have to write a bunch of test cases? :)

@fantasai
Copy link
Contributor

fantasai commented Mar 14, 2017

I can pass that on. :) Note there are use cases for allowing more customization of these elements -- see w3c/csswg-drafts#610 . So I don't think the HTML spec should disallow additional properties applying, just set a minimum for the ones we know need to apply. That way, we can, over time, try to accommodate more of the use cases.

@domenic
Copy link
Member

domenic commented Mar 14, 2017

Thanks. I'm about to post a new draft of #2298 and maybe you can help with the phrasing there.

domenic added a commit that referenced this issue Mar 14, 2017
@fantasai
Copy link
Contributor

The Gecko code for BR is here: https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/layout/generic/BRFrame.cpp#87
It looks up the line-height (which usually depends on font-size and font-size-adjust) for sizing. It seems to also be affected by certain quirks and font size inflation (iirc that's a mobile thing). The clear property definitely applies, so the list is clear and line-height (including anything that influences the used value of line-height), plus quirks.

@fantasai
Copy link
Contributor

... and the Gecko code for WBR appears to construct a base-class nsFrame, which I didn't even know happened ever. Its reflow method returns a zero-sized box. I'm not sure how the line-breaking opportunity is introduced, but I'm guessing no CSS properties apply.

@zcorpan
Copy link
Member Author

zcorpan commented Mar 15, 2017

Thanks, that helps. I'm sure clear is necessary for web compat.

This bit also seems interesting:

... Additionally, we suppress breaks from BR inside
  // of ruby frames. ...

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4947

WebKit/Chromium break the line; Gecko doesn't; IE and Edge 13 don't break but insert some vertical space instead?

@fantasai
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

7 participants