Bug 107380 - Specifying the width/height of an input element modifies its margin
Summary: Specifying the width/height of an input element modifies its margin
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: BrowserCompat, InRadar
Depends on:
Blocks: 110007
  Show dependency treegraph
 
Reported: 2013-01-19 14:25 PST by Mike Sherov
Modified: 2020-04-04 15:49 PDT (History)
31 users (show)

See Also:


Attachments
To check layout test result (3.36 KB, patch)
2019-05-28 07:34 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (382.77 KB, application/zip)
2019-05-28 08:18 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (364.07 KB, application/zip)
2019-05-28 08:24 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (335.46 KB, application/zip)
2019-05-28 08:29 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (330.36 KB, application/zip)
2019-05-28 08:44 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews213 for win-future (3.94 MB, application/zip)
2019-05-28 08:59 PDT, EWS Watchlist
no flags Details
update mac, ios layout test result (110.89 KB, patch)
2019-05-29 06:51 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (388.39 KB, application/zip)
2019-05-29 07:36 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (375.89 KB, application/zip)
2019-05-29 07:42 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (362.93 KB, application/zip)
2019-05-29 07:50 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews215 for win-future (3.99 MB, application/zip)
2019-05-29 07:54 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (478.46 KB, application/zip)
2019-05-29 08:05 PDT, EWS Watchlist
no flags Details
Rebasing regression test results is needed (3.34 KB, patch)
2019-05-29 16:18 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
To check Layout test result (1.22 MB, patch)
2020-03-08 05:40 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
To check layout test results (1.21 MB, patch)
2020-03-08 05:51 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
To check mac-wk2 ews result (1.19 MB, patch)
2020-03-08 07:35 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
To check mac-wk1 layout test result (1.22 MB, patch)
2020-03-08 10:56 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Update layout results for mac-wk1, mac-wk2 and ios-api (1.23 MB, patch)
2020-03-09 19:18 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Check layout test result for mac-wk1, mac0wk2, api-ios and api-mac (1.23 MB, patch)
2020-03-12 09:01 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Check ios-wk2 ews result (2.27 MB, patch)
2020-03-13 22:02 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Check ews result (2.26 MB, patch)
2020-03-14 17:10 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Check ews results (2.25 MB, patch)
2020-03-14 19:09 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Mark win TestExpectations temporarily to check ews result (2.27 MB, patch)
2020-03-19 08:16 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Mark win TestExpectations with [Pass Failure] temporarily to check ews result (2.27 MB, patch)
2020-03-19 09:02 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase patch (2.17 MB, patch)
2020-03-31 09:04 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase patch to rebase on AppleWin port (2.18 MB, patch)
2020-03-31 18:37 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase api test results for api-ios (2.18 MB, patch)
2020-04-01 09:29 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase layout test result on AppleWin port (3.07 MB, patch)
2020-04-02 09:44 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase layout test result on AppleWin port (3.08 MB, patch)
2020-04-03 07:37 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase layout test result on AppleWin port (3.07 MB, patch)
2020-04-03 08:17 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase layout test result on AppleWin port (3.08 MB, patch)
2020-04-03 18:22 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rebase patch (3.08 MB, patch)
2020-04-03 19:08 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch for review (3.08 MB, patch)
2020-04-03 21:53 PDT, Joonghun Park
jh718.park: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Sherov 2013-01-19 14:25:40 PST
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.
Comment 1 Mike Sherov 2013-01-19 14:37:05 PST
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/
Comment 2 Takashi Sakamoto 2013-01-20 21:02:33 PST
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
Comment 3 Mike Sherov 2013-01-21 05:40:42 PST
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?
Comment 4 Mike West 2013-02-25 06:07:20 PST
+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.
Comment 5 Dave Hyatt 2013-02-25 11:12:43 PST
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).
Comment 6 Mike West 2013-02-25 11:24:12 PST
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. :/
Comment 7 Mike West 2013-02-25 11:26:06 PST
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?
Comment 8 Mike Sherov 2013-02-25 13:08:02 PST
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!
Comment 9 GU Yiling 2013-05-04 23:21:58 PDT
@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?
Comment 10 Mike Sherov 2013-10-17 06:21:12 PDT
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
Comment 11 Mike Sherov 2014-09-10 13:24:10 PDT
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.
Comment 12 Derk-Jan Hartman 2017-09-04 13:29:19 PDT
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)
Comment 13 Radar WebKit Bug Importer 2017-09-06 09:43:09 PDT
<rdar://problem/34281872>
Comment 14 Simon Pieters 2019-05-17 06:28:39 PDT
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.
Comment 15 Joonghun Park 2019-05-28 07:34:14 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-05-28 08:18:28 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2019-05-28 08:18:30 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-05-28 08:24:10 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-05-28 08:24:12 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2019-05-28 08:29:14 PDT Comment hidden (obsolete)
Comment 21 EWS Watchlist 2019-05-28 08:29:16 PDT Comment hidden (obsolete)
Comment 22 EWS Watchlist 2019-05-28 08:44:37 PDT Comment hidden (obsolete)
Comment 23 EWS Watchlist 2019-05-28 08:44:39 PDT Comment hidden (obsolete)
Comment 24 EWS Watchlist 2019-05-28 08:58:59 PDT Comment hidden (obsolete)
Comment 25 EWS Watchlist 2019-05-28 08:59:01 PDT Comment hidden (obsolete)
Comment 26 Joonghun Park 2019-05-29 06:51:57 PDT Comment hidden (obsolete)
Comment 27 EWS Watchlist 2019-05-29 07:36:00 PDT Comment hidden (obsolete)
Comment 28 EWS Watchlist 2019-05-29 07:36:02 PDT Comment hidden (obsolete)
Comment 29 EWS Watchlist 2019-05-29 07:42:53 PDT Comment hidden (obsolete)
Comment 30 EWS Watchlist 2019-05-29 07:42:55 PDT Comment hidden (obsolete)
Comment 31 EWS Watchlist 2019-05-29 07:50:05 PDT Comment hidden (obsolete)
Comment 32 EWS Watchlist 2019-05-29 07:50:07 PDT Comment hidden (obsolete)
Comment 33 EWS Watchlist 2019-05-29 07:54:04 PDT Comment hidden (obsolete)
Comment 34 EWS Watchlist 2019-05-29 07:54:06 PDT Comment hidden (obsolete)
Comment 35 EWS Watchlist 2019-05-29 08:05:02 PDT Comment hidden (obsolete)
Comment 36 EWS Watchlist 2019-05-29 08:05:04 PDT Comment hidden (obsolete)
Comment 37 Joonghun Park 2019-05-29 16:18:22 PDT Comment hidden (obsolete)
Comment 38 Joonghun Park 2019-05-29 21:27:54 PDT
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.
Comment 39 Mike Sherov 2020-01-21 07:24:38 PST
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 :-)
Comment 40 Ryosuke Niwa 2020-01-21 23:52:53 PST
(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.
Comment 41 Joonghun Park 2020-01-22 00:17:08 PST
(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 42 Joonghun Park 2020-03-08 05:40:51 PDT Comment hidden (obsolete)
Comment 43 Joonghun Park 2020-03-08 05:51:12 PDT Comment hidden (obsolete)
Comment 44 Joonghun Park 2020-03-08 07:35:53 PDT Comment hidden (obsolete)
Comment 45 Joonghun Park 2020-03-08 10:56:05 PDT Comment hidden (obsolete)
Comment 46 Simon Fraser (smfr) 2020-03-09 10:03:11 PDT
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?
Comment 47 Mike Sherov 2020-03-09 10:14:50 PDT
> 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.
Comment 48 Simon Fraser (smfr) 2020-03-09 10:24:16 PDT
(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.
Comment 49 Joonghun Park 2020-03-09 19:18:13 PDT Comment hidden (obsolete)
Comment 50 Joonghun Park 2020-03-09 20:07:41 PDT
(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.
Comment 51 Joonghun Park 2020-03-12 09:01:39 PDT Comment hidden (obsolete)
Comment 52 Joonghun Park 2020-03-13 22:02:39 PDT Comment hidden (obsolete)
Comment 53 Joonghun Park 2020-03-14 17:10:34 PDT Comment hidden (obsolete)
Comment 54 Joonghun Park 2020-03-14 19:09:19 PDT Comment hidden (obsolete)
Comment 55 Joonghun Park 2020-03-19 08:16:55 PDT Comment hidden (obsolete)
Comment 56 Joonghun Park 2020-03-19 09:02:35 PDT Comment hidden (obsolete)
Comment 57 Joonghun Park 2020-03-31 09:04:51 PDT Comment hidden (obsolete)
Comment 58 Joonghun Park 2020-03-31 18:37:47 PDT Comment hidden (obsolete)
Comment 59 Joonghun Park 2020-04-01 09:29:08 PDT Comment hidden (obsolete)
Comment 60 Joonghun Park 2020-04-02 09:44:43 PDT Comment hidden (obsolete)
Comment 61 Joonghun Park 2020-04-03 07:37:43 PDT Comment hidden (obsolete)
Comment 62 Joonghun Park 2020-04-03 08:17:08 PDT Comment hidden (obsolete)
Comment 63 Joonghun Park 2020-04-03 18:22:32 PDT Comment hidden (obsolete)
Comment 64 Joonghun Park 2020-04-03 19:08:20 PDT Comment hidden (obsolete)
Comment 65 Joonghun Park 2020-04-03 21:53:05 PDT
Created attachment 395433 [details]
Patch for review
Comment 66 Joonghun Park 2020-04-04 15:46:02 PDT
Could you please review this patch?
All the ews bots shows green.