Repro steps: 1. Please http://jsfiddle.net/JZSMt/4/ 2. I query getComputedStyle, and retrieve the height property of the input element without a specified height, and then set that value back in as the height style attribute value. What happens: the marginTop and marginBottom values change from "2px" to "0px". What I expect: the margin to remain unchanged. This behavior deviates from FF, IE9+, and Opera, but is present in Safari 6.0 and Chrome as of this writing.
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.
Created attachment 370736 [details] To check layout test result
Comment on attachment 370736 [details] To check layout test result Attachment 370736 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12308337 Number of test failures exceeded the failure limit.
Created attachment 370741 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 370736 [details] To check layout test result Attachment 370736 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12308341 Number of test failures exceeded the failure limit.
Created attachment 370742 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 370736 [details] To check layout test result Attachment 370736 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12308321 Number of test failures exceeded the failure limit.
Created attachment 370743 [details] Archive of layout-test-results from ews113 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 370736 [details] To check layout test result Attachment 370736 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12308350 Number of test failures exceeded the failure limit.
Created attachment 370744 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Comment on attachment 370736 [details] To check layout test result Attachment 370736 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12308421 Number of test failures exceeded the failure limit.
Created attachment 370748 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 370841 [details] update mac, ios layout test result
Comment on attachment 370841 [details] update mac, ios layout test result Attachment 370841 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12317706 Number of test failures exceeded the failure limit.
Created attachment 370843 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 370841 [details] update mac, ios layout test result Attachment 370841 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12317716 Number of test failures exceeded the failure limit.
Created attachment 370844 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 370841 [details] update mac, ios layout test result Attachment 370841 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12317691 Number of test failures exceeded the failure limit.
Created attachment 370845 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 370841 [details] update mac, ios layout test result Attachment 370841 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12317748 Number of test failures exceeded the failure limit.
Created attachment 370846 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 370841 [details] update mac, ios layout test result Attachment 370841 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12317734 Number of test failures exceeded the failure limit.
Created attachment 370848 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Created attachment 370896 [details] Rebasing regression test results is needed
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.
Created attachment 392948 [details] To check Layout test result
Created attachment 392949 [details] To check layout test results
Created attachment 392951 [details] To check mac-wk2 ews result
Created attachment 392960 [details] To check mac-wk1 layout test result
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.
Created attachment 393109 [details] Update layout results for mac-wk1, mac-wk2 and ios-api
(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 393380 [details] Check layout test result for mac-wk1, mac0wk2, api-ios and api-mac
Created attachment 393573 [details] Check ios-wk2 ews result
Created attachment 393598 [details] Check ews result
Created attachment 393603 [details] Check ews results
Created attachment 393979 [details] Mark win TestExpectations temporarily to check ews result
Created attachment 393982 [details] Mark win TestExpectations with [Pass Failure] temporarily to check ews result
Created attachment 395043 [details] Rebase patch
Created attachment 395119 [details] Rebase patch to rebase on AppleWin port
Created attachment 395177 [details] Rebase api test results for api-ios
Created attachment 395272 [details] Rebase layout test result on AppleWin port
Created attachment 395375 [details] Rebase layout test result on AppleWin port
Created attachment 395376 [details] Rebase layout test result on AppleWin port
Created attachment 395426 [details] Rebase layout test result on AppleWin port
Created attachment 395429 [details] Rebase patch
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/
https://github.com/WebKit/WebKit/pull/1372
*** 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 :-)