Bug 107380

Just one more piece of information, if I query the margin, then specify the margin with the queried value, then query the height, then specify the height with the queried value, everything works as expected: http://jsfiddle.net/JZSMt/5/

I think, this is caused by StyleResolver: static void addIntrinsicMargins(RenderStyle* style) { .... if (style->height().isAuto()) { if (style->marginTop().quirk()) style->setMarginTop(Length(intrinsicMargin, Fixed)); if (style->marginBottom().quirk()) style->setMarginBottom(Length(intrinsicMargin, Fixed)); } } and StyleResolver::adjustRenderStyle(): if (e && e->isFormControlElement() && style->fontSize() >= 11) { if (!e->hasTagName(inputTag) || !static_cast<HTMLInputElement*>(e)->isI\ mageButton()) addIntrinsicMargins(style); } Since input's default height is auto and margin is 0__qem, its margin top will be "2px" (intrinsicMargin = 2). After setting input's height, e.g. 16px, the height is not auto and no addIntrinsicMargins is invoked. The marginTop is "0px". Best regards, Takashi Sakamoto

Thanks Takashi! So then is this a bug or expected behavior? That seems to be the exact code path, however I'm not sure exactly WHY the code's doing that. Is there some part of the spec I'm missing that dictates this behavior?

+dhyatt It doesn't look like anyone has touched this code since dhyatt@ checked it in, many years ago. :) dhyatt: Would you mind giving some background on this feature? From the commit log, it appears that the purpose is to make the focus ring more visually appealing by spacing it out a bit from the element, but I can imagine that there are other reasons as well. I'm hopeful that we can find a less fragile mechanism of achieving those goals.

The purpose of intrinsic margins is to try to prevent adjacent controls from butting up against one another. Especially with rounded controls this looks terrible. The reason you see the values change is that we only set intrinsic margins when we think it won't disrupt the layout of the page. If the author specifies an explicit height/width on the control, then we assume the designer is needing pixel-precise control, and so we turn the margins off to avoid disrupting the page layout. This feature was originally motivated by cases like Google's front page, which had two buttons that butted right up against one another. They have spacing now on the modern version of the page, but in the past they did not. This happens all over the place on the Web as well, especially with text fields next to buttons. They end up looking terrible on OS X especially (they look bad on Windows too, just not quite as bad).

From IRC (thanks, dhyatt!): 19:57 <dhyatt> mkwst: yeah i know about that code 19:57 <dhyatt> mkwst: so in order to make form controls look nice and not butt up against each other we have this feature called intrinsic margins. 19:58 <dhyatt> the basic idea is that - if we can detect it won't screw up the layout of the web page - we will add in intrinsic margins to provide a nice separation between adjacent controls the classic example years ago was google's home page with its two buttons that were right next to each other which on mac especially looked terrible since you never want aqua controls right up against one another (looks bad on windows too, just less bad). 19:59 <mkwst> dhyatt: ok. so it's not just the focus ring, but layout in general. 19:59 <dhyatt> so the basic idea is that if the author doesn't know the iwdth/height of the control (e.g., he didn't specify it explicitly) then it's safe to add in some margins but if the author said "i want this control to be 100px wide" then adding in margins is iffy since you could disrupt the author's intended page layout so that's the motivation behind sometimes turning intrinsic margins off. 20:00 <esprehn_> mkwst: form controls of full of these lies. We also tell you it has a 2 pixel beveled border and a grey background 20:00 <mkwst> esprehn_: lovely. :) 20:00 <esprehn_> getComputedStyle(input) is not super useful unless you styled it yourself 20:00 <dhyatt> the intrinsic margin feature is really nice 20:01 <dhyatt> like if you turn it off you would notice pretty fast on web pages. especially on mac. 20:01 <esprehn_> dhyatt: does Gecko do this? 20:01 <dhyatt> no this is something i invented 20:01 <dhyatt> i don't think any other browser does it 20:01 <esprehn_> seems like it would cause wrapping of controls that are inside in app toolbars. maybe that's what I've seen before. 20:01 <dhyatt> it shouldn't 20:01 <dhyatt> i mean, the margin is only added in if the control size is unspecified, so the control size was already going to vary across browsers and platforms. if the author specifies the control size, then the margins turn off 20:03 <dhyatt> i don't recall ever seeing a bug where the intrinsic margins were to blame for disrupted page layout 20:04 <dhyatt> but if those bugs do exist, i would be willing to revisit 20:04 <esprehn_> nah, this behavior is sensible, it's just not documented 20:04 <dhyatt> but in the absence of those bugs, i'd recommend leaving the feature in, since controls with no gaps at all between them look really icky 20:04 <dhyatt> and it happens way too much 20:04 <mkwst> Right, but that bit is surprising for authors. I don't think it breaks anything in a big way, but jQuery has seen bugs filed. 20:04 <esprehn_> we could fix this with -webkit-input-spacing: auto or something 20:04 <mkwst> I don't think removing the margins is a good idea. 20:05 <esprehn_> then you'd see it in the inspector and could google search for what that was 20:05 <dhyatt> esprehn_: it needs to be margins though. if the author specifies an explicit margin, it needs to take precedence 20:05 <mkwst> But the current behavior is surprising. 20:05 <dhyatt> i mean, it's not lying. it's genuinely telling you the margin is there, right? what's surprising is that it changed. 20:06 <mkwst> That's what I mean. 20:06 <dhyatt> i mean, we could do something like not get rid of intrinsic margins if height/width are set dynamically 20:06 <dhyatt> but that seems iffy to me 20:06 <mkwst> Hrm. I agree. :/

Note that this particular test case would be mitigated completely if there was a mechanism to set and retrieve "auto" as the element's height/width. Mike: What's the use-case you folks are running into here?

So, the use case is: I am changing an element to "position: absolute" so I can animate its clip property. In order to do that, I need to lock in the element's width, in case it was width auto when it was relative or static and expanded to fill its container. If the element happens to be an input, this then changes the margins of the input. I can work around this in 1 of 2 ways: 1. set the margin before setting the width, but this is less than ideal because I can't get the *computed* margin, only the used margin (from getComputedStyle), so I lose any auto-y goodness. 2. I can detect that it's an input, and assume it's got this intrinsic thing going on, but that seems like it comes with its own set of edge cases. I currently have done #1 as my workaround. I'll respond to the IRC conversation in another comment. Thanks again for looking into this, guys!

@dhyatt Hi Hyatt, I have a further question here. Is '__qem' just designed for such use cases like preventing 'adjacent controls from butting up against one another'? Are there any other use cases in which __qem could be useful? I noticed in WebKit's user agent stylesheet, __qem is used in other element's margin settings. Are all those used to add some margin to elements while authors don't control their style precisely?

Just updating information here from Blink: it seems like a few userland bugs actually are being reported against this feature: http://code.google.com/p/chromium/issues/detail?id=128306

This is now fixed in Blink. https://code.google.com/p/chromium/issues/detail?id=128306 Webkit is now the only browser engine that exhibits this quirk.

I ran into this inconsistency again today. Safari is now the only browser that has this behavior and it's long since fixed in Blink, as can be seen from the Chromium issue. It is also HIGHLY confusing, as there is even an explicit User Agent style for input elements that says "margin: 0". If you look in the inspector, this UA style statement is non-striken, and thus the 'prevailing' rule, yet if you look at the computed style the margin is suddenly 2px. Can we please fix this ???? (Checked and still a problem in STP as well)

<rdar://problem/34281872>

I think this behavior should be removed from WebKit. Differences in rendering of form controls is high on the list of things that are annoying to web developers, and here WebKit is deliberately being different from other browser engines in a weird/magic way. I learned today that normalize.css explicitly sets the margin to 0 because of this bug. https://github.com/csstools/normalize.css/blob/master/normalize.css#L168 Web developers shouldn't need to deal with this. We need interoperability.

This change will be in pending state until august/september, when the patches which might cause compatibility issues can be landed safely, according to Simon's comment in #webkit irc.

As a curious spectator, did a fix for this ever land? My interest is sentimental, as it's the last blocker to the meta issue: "bugs that jQuery works around", which I've contributed to and watched for years :-) I have an old bottle of champagne waiting to be opened once the quirky em margin bug is fixed :-)

(In reply to Mike Sherov from comment #39) > As a curious spectator, did a fix for this ever land? My interest is > sentimental, as it's the last blocker to the meta issue: "bugs that jQuery > works around", which I've contributed to and watched for years :-) No. The bug is still open.

(In reply to Mike Sherov from comment #39) > As a curious spectator, did a fix for this ever land? My interest is > sentimental, as it's the last blocker to the meta issue: "bugs that jQuery > works around", which I've contributed to and watched for years :-) > > I have an old bottle of champagne waiting to be opened once the quirky em > margin bug is fixed :-) I just checked that the code related to this issue was moved to StyleAdjuster from StyleResolver by r252308. It seems that with applying the previous change(https://bugs.webkit.org/attachment.cgi?id=370896&action=review), layout test results should be updated also, and after that I have to get a review if it is ok to land in aspect of compatibility issue. For now, I will upload a new patch with the updated layout test result for it.

Comment on attachment 392960 [details] To check mac-wk1 layout test result View in context: https://bugs.webkit.org/attachment.cgi?id=392960&action=review > Source/WebCore/ChangeLog:11 > + Currently, all form control elements except image button > + get changed margin if width/height is specified by css. > + For interoperability with other browsers, > + remove this behavior. Do we understand this history of this behavior, and why we think removing it is OK?

> Do we understand this history of this behavior, and why we think removing it is OK? Comments 1-9 lay out the history in detail. The short version is: > 19:58 <dhyatt> the basic idea is that - if we can detect it won't screw up the layout of the web page - we will add in intrinsic margins to provide a nice separation between adjacent controls the classic example years ago was google's home page with its two buttons that were right next to each other which on mac especially looked terrible since you never want aqua controls right up against one another (looks bad on windows too, just less bad). > 19:59 <mkwst> dhyatt: ok. so it's not just the focus ring, but layout in general. > 19:59 <dhyatt> so the basic idea is that if the author doesn't know the iwdth/height of the control (e.g., he didn't specify it explicitly) then it's safe to add in some margins but if the author said "i want this control to be 100px wide" then adding in margins is iffy since you could disrupt the author's intended page layout so that's the motivation behind sometimes turning intrinsic margins off. > 20:01 <esprehn_> dhyatt: does Gecko do this? > 20:01 <dhyatt> no this is something i invented > 20:01 <dhyatt> i don't think any other browser does it It has also been removed in Chrome for a few years now, and was the source of several layout measurement issues in jQuery UI.

(In reply to Mike Sherov from comment #47) > It has also been removed in Chrome for a few years now, and was the source > of several layout measurement issues in jQuery UI. The changelog should summarize this history.

(In reply to Simon Fraser (smfr) from comment #48) > (In reply to Mike Sherov from comment #47) > > It has also been removed in Chrome for a few years now, and was the source > > of several layout measurement issues in jQuery UI. > > The changelog should summarize this history. I'm currently rebasing the layout test results for this change for several ports. Especially for win port, I need to configure win port environment, so it will take some time but I will eventually get there. In the next patchset I will update the changelog to include the history also as you commented. My plan is to pass all the ews bots within this week.

Created attachment 395433 [details] Patch for review

Could you please review this patch? All the ews bots shows green.

This change seems like a good idea, and I guess we really should return to it and land it. Note that almost all of the patch is all the rebased layout tests, the actual code change is tiny. Turning this off with a feature flag might be better than just removing the code entirely, at first. We don’t know if iOS apps, for example, accidentally rely on this for a good layout. Not sure what other considerations apply.

Might also be nice if there was a WPT test that shows the problem, since it’s a real compatibility issue. It’s kind of disappointing that removing this doesn’t cause even a single WPT test to pass.

Since it’s likely a real interoperability issue, I mean.

(In reply to Darin Adler from comment #68) > Might also be nice if there was a WPT test that shows the problem, since > it’s a real compatibility issue. It’s kind of disappointing that removing > this doesn’t cause even a single WPT test to pass. I think this should result in https://wpt.fyi/results/css/cssom/cssstyledeclaration-setter-form-controls.html passing.

(In reply to Darin Adler from comment #67) > This change seems like a good idea, and I guess we really should return to > it and land it. > > Note that almost all of the patch is all the rebased layout tests, the > actual code change is tiny. > > Turning this off with a feature flag might be better than just removing the > code entirely, at first. We don’t know if iOS apps, for example, > accidentally rely on this for a good layout. > > Not sure what other considerations apply. I agree that a feature flag would be better to reduce risk.

(In reply to Aditya Keerthi from comment #71) > (In reply to Darin Adler from comment #67) > > This change seems like a good idea, and I guess we really should return to > > it and land it. > > > > Note that almost all of the patch is all the rebased layout tests, the > > actual code change is tiny. > > > > Turning this off with a feature flag might be better than just removing the > > code entirely, at first. We don’t know if iOS apps, for example, > > accidentally rely on this for a good layout. > > > > Not sure what other considerations apply. > > I agree that a feature flag would be better to reduce risk. Will this be landed under a feature flag soon?

I don’t see a patch yet; is someone working on it?

The following "contain" tests also fail due to the intrinsic margins: imported/w3c/web-platform-tests/css/css-contain/contain-size-button-001.html imported/w3c/web-platform-tests/css/css-contain/contain-size-button-002.html in bug 238587, I traced it all the way back to r10888 see: "This was introduce at r10888 with the idea of "With this landing both <input type=button> and <button> will now look like OS X-style widgets." and maybe we should check if this is still the desired approach for our form controls."

*** Bug 241181 has been marked as a duplicate of this bug. ***

Pull Request from Ziran: https://github.com/WebKit/WebKit/pull/1236/

*** Bug 243799 has been marked as a duplicate of this bug. ***

Pull request: https://github.com/WebKit/WebKit/pull/3205

Committed 253424@main (fc87a93fd5ce): <https://commits.webkit.org/253424@main> Reviewed commits have been landed. Closing PR #3205 and removing active labels.

Follow-up rebaselining: https://commits.webkit.org/253427@main

(In reply to Chris Dumez from comment #81) > Follow-up rebaselining: https://commits.webkit.org/253427@main Another follow-up test fix: https://commits.webkit.org/253429@main

Congrats! 🍾 This fixes the final bug of "issues that jQuery works around". 9 years in the making :-)