Closed Bug 1471894 Opened 6 years ago Closed 6 years ago

Absolute positioned blocks with auto margins never update their UsedMargin property

Categories

(Core :: Layout: Positioned, defect, P2)

61 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 + wontfix
firefox62 + fixed
firefox63 + fixed

People

(Reporter: seti40s, Assigned: bradwerth)

References

()

Details

(4 keywords)

Attachments

(5 files)

Attached image Bild_gescrollt_3.jpg
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180621125625

Steps to reproduce:

You can see the problem during scrolling down in the specified homepageexamples

http://www.checkbier.at/dsgvo.html or
http://www.f-soft.at/agb.html


Actual results:

The complete Hamburger Button and Menuitem is moving to the right. 

This Problem das not exist in Opera, IE, Edge or Chrome, I have tried to verify several times


Expected results:

The Hamburger Button and Menuitem should stay on the same position before scrolling down
Component: Untriaged → Layout: R & A Pos
Product: Firefox → Core
So they're setting the left property with javascript to 400+px. If you set that to zero manually suddenly you get the right rendering.

They come up with zero as an answer for other browsers, and the answer to why lies in this file: http://www.checkbier.at/res/x5engine.deferrable.js?16-0-4-1

This is a regression, btw, because 57 shows up correctly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=908749c3b8584dcbe7d4dcccf0f4c06921eee68d&tochange=f5d5d6a09965236f1f594390ebc772be986c1b1c

This seems like may affect a fair amount of e-commerce websites looking at the library.

Looks like this page is looking at the margin of the header to figure out the right position to center it, and we fail to return the same as other browsers.

Brad, Daniel, could you please take a look?
Blocks: 1298008
Flags: needinfo?(dholbert)
Flags: needinfo?(bwerth)
This is a tough one. I've convinced myself that the value we wish would be zero is coming from some jQuery: "p=a.parent().offset().left"

The jQuery offset() function does appear to call getBoundingClientRect(), according to https://searchcode.com/codesearch/view/79669847/

So this is following a code path that matches the bug found in our regression range. It may be that the library could be made correct if the code "g=function(){return-o.scrollLeft()+l+p}" was changed to omit the addition of the "p" value, but it's impossible to discern the semantics of that with the mini-fied JS.

I'll see if I can find an un-mini-fied version of the library and propose a semantic fix to the library authors.
I think we need to figure out why other browsers don't get the same result, do they fail your WPT test?
Chrome fails the WPT test I created because they don't implement getBoxQuads. Their official status says that they implemented GeometryUtils in https://www.chromestatus.com/feature/6015941766807552, but a posting in their forums clarifies that they don't actually implement getBoxQuads: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/cAiTOdSGyes

From what I learn investigating this library's behavior, I probably can create a new WPT test that hits getBoundingClientRect(), which Chrome does support, and that will tell us more about the differing behavior.
(In reply to Brad Werth [:bradwerth] from comment #5)
> Chrome fails the WPT test I created because they don't implement
> getBoxQuads. Their official status says that they implemented GeometryUtils
> in https://www.chromestatus.com/feature/6015941766807552, but a posting in
> their forums clarifies that they don't actually implement getBoxQuads:

Sorry, bad link. The comment I'm referencing is https://groups.google.com/a/chromium.org/d/msg/blink-dev/cAiTOdSGyes/aTlbD6u7CAAJ
Flags: needinfo?(dholbert)
I'll take the bug and sort it out.
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
New theory: the value is getting exposed through a jQuery call to retrieve the marginLeft property on the computed style. I can't extract the JS stack from my debugger to figure out the exact path, but when I zero out the value, we get the desired behavior. However, in all the tests I can think to try, FF and Chrome returns the same values for marginLeft on elements with margin: auto. So that might indicate there is a UA-specific behavior in either jQuery or the Website X5 library.

jQuery does some feature detection about "reliableMarginLeft", and FF is currently marked as false: https://github.com/jquery/jquery/blob/9a5b3b6ed0803d816984718de23d6af451260c89/test/unit/support.js#L177
Chrome is marked as reliableMarginLeft: true.

I attempted a UA spoof to a Chrome string, but it didn't change the test site behavior in FF. I'm going to continue to investigate how jQuery does browser detection and see if I can confirm that this reliableMarginLeft property is the culprit.
(In reply to Brad Werth [:bradwerth] from comment #8)
> New theory: the value is getting exposed through a jQuery call to retrieve
> the marginLeft property on the computed style. I can't extract the JS stack
> from my debugger to figure out the exact path, but when I zero out the
> value, we get the desired behavior. However, in all the tests I can think to
> try, FF and Chrome returns the same values for marginLeft on elements with
> margin: auto. So that might indicate there is a UA-specific behavior in
> either jQuery or the Website X5 library.
> 
> jQuery does some feature detection about "reliableMarginLeft", and FF is
> currently marked as false:
> https://github.com/jquery/jquery/blob/
> 9a5b3b6ed0803d816984718de23d6af451260c89/test/unit/support.js#L177
> Chrome is marked as reliableMarginLeft: true.
> 
> I attempted a UA spoof to a Chrome string, but it didn't change the test
> site behavior in FF. I'm going to continue to investigate how jQuery does
> browser detection and see if I can confirm that this reliableMarginLeft
> property is the culprit.

Ugh, nice catch! Yeah, looks definitely related... What they do is falling back to a getBoundingClientRect() calculation:

  https://github.com/jquery/jquery/blob/e743cbd28553267f955f71ea7248377915613fd9/src/css.js#L394

And our getBoundingClientRect().left is exactly what they shift the bar. This is really annoying. :((
It looks like reliableMarginLeft should be set to true for FF (https://github.com/jquery/jquery/blob/821bf34353a6baf97f7944379a6459afb16badae/src/css/support.js#L33), in computeStyleTests(). The link I found in comment 8 was to expected test results. Presumably some of those tests are now an unexpected pass for FF.

I was able to get the JS stack that is hitting nsComputedDOMStyle::DoGetMarginLeftWidth() and then GetUsedMargin():

0 b() ["http://www.f-soft.at/res/jquery.js?16-0-4-1":3]
1 pixelMarginRight() ["http://www.f-soft.at/res/jquery.js?16-0-4-1":3]
    this = [object Object]
2 Ma(a = [object HTMLElement], b = "paddingBottom", c = [object CSS2Properties]) ["http://www.f-soft.at/res/jquery.js?16-0-4-1":3]
3 css(a = [object HTMLElement], b = "paddingBottom") ["http://www.f-soft.at/res/jquery.js?16-0-4-1":3]
    this = function(a,b){return new r.fn.init(a,b)}
4 css/<(a = [object HTMLElement], b = "padding-bottom") ["http://www.f-soft.at/res/jquery.js?16-0-4-1":3]
5 S(a = [object Object], b = function(a,b,c){var d,e,f={},g=0;if(r.isArray(b)){for(d=La(a),e=b.length;e>g;g++)f[b[g]]=r.css(a,b[g],!1,d);return f}return void 0!==c?r.style(a,b,c):r.css(a,b)}, c = "padding-bottom", d = undefined, e = false) ["http://www.f-soft.at/res/jquery.js?16-0-4-1":3]
6 css(a = "padding-bottom") ["http://www.f-soft.at/res/jquery.js?16-0-4-1":3]
    this = [object Object]
7 anonymous(e = function(a,b){return new r.fn.init(a,b)}, t = [object Object]) ["http://www.f-soft.at/res/x5engine.deferrable.js?16-0-4-1":3]
8 <TOP LEVEL> ["http://www.f-soft.at/res/x5engine.deferrable.js?16-0-4-1":3]

That last call in mini-fied jQuery is r.extend(o,{pixelPosition:function(){return b(),c},boxSizingReliable:function(){return b(),e},pixelMarginRight:function(){return b(),f},reliableMarginLeft:function(){return b(),g}}) which looks a lot like the call to computeStyleTests. That would mean that my interception of the call and setting left to 0 is setting reliableMarginLeft to FALSE, which makes the jQuery marginLeft call go through getBoundingClientRect and that makes the site work. That leads me to a new theory that the library Website X5 Engine is making its own assumptions about FF capabilities, and not relying on the jQuery feature detection.
Alright, I've figured out the difference between Firefox behavior and Chrome behavior. The difference is in the reported value of margin-left for a block with margin: auto. FF returns 0, and Chrome returns a computed pixel value. I'll build a test then work on the fix.
This reduced testcase shows the difference between Firefox behavior and the presumably correct Chrome and Safari behavior. In this example, we are providing computed style values for left and margin-left that are the opposite values returned by the other browsers.
Summary: Problems with Hamburger-Button bars in Windows → Absolute positioned blocks with auto margins never update their UsedMargin property
Attachment #8991486 - Flags: review?(dholbert)
Attachment #8991711 - Flags: review?(dholbert)
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265266

Yikes, just noticed this in my review queue -- sorry for the delay! It slipped behind a few other things and drived off my radar as something recent that needed circling back to.

::: layout/generic/ReflowInput.cpp:1236
(Diff revision 2)
> -      // XXX FIXME (or does CalculateBlockSideMargins do this?)
> -      start = 0;  // just ignore
> +      // We set this to 0 for now, and fix it up in InitAbsoluteConstraints,
> +      // which is caller of this function, via CalculateHypotheticalPosition.
> +      start = 0;

Let's wrap the second line in parens here (and drop the comma at the end of the first line), to make the grouping between the last two clauses unambiguous.

(Right now it's easy & grammatically-valid to view the second clause as a standalone interjection -- i.e. to misunderstand this sentence as saying:
 "...fix it up in InitAbsoluteConstraints [...] via CalculateHypotheticalPosition.")

::: layout/generic/ReflowInput.cpp:1245
(Diff revision 2)
> -      // XXX FIXME (or does CalculateBlockSideMargins do this?)
> -      end = 0;  // just ignore
> +      // We set this to 0 for now, and fix it up in InitAbsoluteConstraints,
> +      // which is caller of this function, via CalculateHypotheticalPosition.

Same here.

::: layout/generic/ReflowInput.cpp:1959
(Diff revision 2)
> +  // Update the UsedMargin property if we were tracking it already.
> +  nsMargin* propValue = mFrame->GetProperty(nsIFrame::UsedMarginProperty());
> +  if (propValue) {
> +    *propValue = marginInOurWM.GetPhysicalMargin(wm);
> +  }

It looks like this is based on this chunk of CalculateBlockSideMargins:

https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/ReflowInput.cpp#2769-2775

In that existing version of this logic, we only do the proptable check & overwriting if we get through a "have auto margins" check -- namely "isAutoStartMargin || isAutoEndMargin".

Can we wrap this code in a similar check? Here, I think it'd be:
  if (marginIStartIsAuto || marginIEndIsAuto ||
      marginBStartIsAuto || marginBEndIsAuto) {

It'd be nice to avoid the property-table lookup and the nsMargin copying for the usual case where it's unnecessary.
Attachment #8991486 - Flags: review?(dholbert) → review+
Comment on attachment 8991711 [details]
Bug 1471894 Part 2: Add a web-platform-test to check computed margin values on auto margin blocks.

https://reviewboard.mozilla.org/r/256654/#review265268

::: testing/web-platform/tests/css/cssom/computed-style-005.html:49
(Diff revision 1)
> +     test(function() {
> +       assert_equals(elemStyle.getPropertyValue("margin-left"), "10px");
> +       assert_equals(elemStyle.getPropertyValue("margin-right"), "10px");

For good measure, would you mind testing the computed values of "left" and "right" here, too, since (per your test here) we were apparently misattributing the margin offsets to those "left" / "right" offsets?

(Right now Nightly renders your testcase attachment with the sentence: "left is 10px and margin-left is 0px", whereas Chrome renders it with the values swapped.  I want to be sure we're fixing the computed "left" value, too -- not just the computed "margin-left".)

::: testing/web-platform/tests/css/cssom/computed-style-005.html:53
(Diff revision 1)
> +
> +     test(function() {
> +       assert_equals(elemStyle.getPropertyValue("margin-left"), "10px");
> +       assert_equals(elemStyle.getPropertyValue("margin-right"), "10px");
> +     }, id + "_computed_margins", {
> +       assert: id + " margins are computed to values"

The logging that this line produces will be a bit confusing right now.

I think it'll produce strings like:
 "absolute margins are computed to values"
and
 "relative values are computed to values"

...which sounds (incorrectly) like it's talking about "absolute" pixel-valued margins vs. "relative" percent-valued margins.

Please clarify to something like:
====
assert: id + "-positioned elements' auto margins can be resolved to nonzero value"
====
Attachment #8991711 - Flags: review?(dholbert) → review+
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265342

::: layout/generic/ReflowInput.cpp:1959
(Diff revision 2)
> +  // Update the UsedMargin property if we were tracking it already.
> +  nsMargin* propValue = mFrame->GetProperty(nsIFrame::UsedMarginProperty());
> +  if (propValue) {
> +    *propValue = marginInOurWM.GetPhysicalMargin(wm);
> +  }

Doesn't that miss the case when our margin goes from auto to non-auto?
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265342

> Doesn't that miss the case when our margin goes from auto to non-auto?

Hmm, I guess it would miss that case right now, yeah.

I was going off of the similar chunk in SizeComputationInput::InitOffsets() (where we check for autoness before updating the property).  In that other InitOffsets code, we have an earlier call to "UpdateProp", which ensures the frame-property exists (and is up-to-date aside from the 'auto' resolution) IFF we need a it to exist.

Perhaps we need something like that UpdateProp call here, too?  Right now it's not clear to me whether/why we're sure that mFrame->GetProperty(nsIFrame::UsedMarginProperty()) will exist when we get to this new InitAbsoluteConstraints clause, in scenarios where we need it to exist here...
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265738

(changing review to r- for the time being because I want to better understand how we ensure this frame property exists when we need it, per previous comment)
Attachment #8991486 - Flags: review+ → review-
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265266

> It looks like this is based on this chunk of CalculateBlockSideMargins:
> 
> https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/ReflowInput.cpp#2769-2775
> 
> In that existing version of this logic, we only do the proptable check & overwriting if we get through a "have auto margins" check -- namely "isAutoStartMargin || isAutoEndMargin".
> 
> Can we wrap this code in a similar check? Here, I think it'd be:
>   if (marginIStartIsAuto || marginIEndIsAuto ||
>       marginBStartIsAuto || marginBEndIsAuto) {
> 
> It'd be nice to avoid the property-table lookup and the nsMargin copying for the usual case where it's unnecessary.

In response your later point that the proposed code doesn't ensure the property exists, I've changed it to route through ::UpdateProp, which handles the three cases of add / update / remove.
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265890

::: layout/generic/ReflowInput.cpp:1986
(Diff revision 3)
> +
> +  // Depending on whether we have auto margins, we will need to either
> +  // add/update, or remove the UsedMarginProperty. The convenience function
> +  // ::UpdateProp handles the logic.
> +  bool needUsedMargin = (marginIStartIsAuto || marginIEndIsAuto ||
> +                         marginBStartIsAuto || marginBEndIsAuto);
> +  ::UpdateProp(mFrame, nsIFrame::UsedMarginProperty(), needUsedMargin,
> +               marginInOurWM.GetPhysicalMargin(wm));

This "needUsedMargin" logic isn't quite correct. This will result in us deleting the margin frame-property even if e.g. we have a percent-valued margin (which we typically store as a resolved value in the frame property).

I imagine this change would break accessors that depend on that working correctly.

(It looks like the one preexisting UpdateProperty(...) call indirectly uses "mMargin.ConvertsToLength()" as its signal, which only returns true for hardcoded lengths like px/em/inch/etc. and calc() expressions using only those values.)

Perhaps we can assume that InitOffsets is called before this method and it will already have done the "UpdateProperty()" dance? I think that actually probably has to be the case, and that's the real answer to emilio's "how will we handle auto --> non-auto conversions".

So I think you don't need to bother calling UpdateProperty after all, and you can perhaps disregard comment 20 & 21... (though it'd be nice to include a code-comment alongside your frame-property update, mentioning how we know that the frame property exists in strictly the scenarios where we need it to exist)
Attachment #8991486 - Flags: review?(dholbert) → review-
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265890

> This "needUsedMargin" logic isn't quite correct. This will result in us deleting the margin frame-property even if e.g. we have a percent-valued margin (which we typically store as a resolved value in the frame property).
> 
> I imagine this change would break accessors that depend on that working correctly.
> 
> (It looks like the one preexisting UpdateProperty(...) call indirectly uses "mMargin.ConvertsToLength()" as its signal, which only returns true for hardcoded lengths like px/em/inch/etc. and calc() expressions using only those values.)
> 
> Perhaps we can assume that InitOffsets is called before this method and it will already have done the "UpdateProperty()" dance? I think that actually probably has to be the case, and that's the real answer to emilio's "how will we handle auto --> non-auto conversions".
> 
> So I think you don't need to bother calling UpdateProperty after all, and you can perhaps disregard comment 20 & 21... (though it'd be nice to include a code-comment alongside your frame-property update, mentioning how we know that the frame property exists in strictly the scenarios where we need it to exist)

Specifically, it looks like InitOffsets is called in the base class's stub constructor (SizeComputationInput::SizeComputationInput()), here:
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/ReflowInput.cpp#159,168-169

...and both ReflowInput constructors invoke that base-class constructor.  So I think InitOffets (and its UpdateProperty() instantiation/deletion) will happen before we get to the InitAbsoluteConstraints code that you're touching.

So I think you can assume that the frame property exists (and needs updating to fill in the 'auto' margin values only) specifically if the frame property exits.
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265890

> Specifically, it looks like InitOffsets is called in the base class's stub constructor (SizeComputationInput::SizeComputationInput()), here:
> https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/ReflowInput.cpp#159,168-169
> 
> ...and both ReflowInput constructors invoke that base-class constructor.  So I think InitOffets (and its UpdateProperty() instantiation/deletion) will happen before we get to the InitAbsoluteConstraints code that you're touching.
> 
> So I think you can assume that the frame property exists (and needs updating to fill in the 'auto' margin values only) specifically if the frame property exits.

You're right; I'll need to unwind this. Upon reviewing the code, it's clear that InitOffsets is called before InitAbsoluteConstraints, at https://searchfox.org/mozilla-central/source/layout/generic/ReflowInput.cpp#2257. I'll instead add a code comment that clarifies how we know that the UsedMarginProperty is available.
Thank you!

(Sorry for not realizing this sooner; I initially had a hunch that the margin might've been getting set up in the frame property earlier, but didn't immediately see where/why.)
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

https://reviewboard.mozilla.org/r/256376/#review265916

Thanks. r=me
Attachment #8991486 - Flags: review?(dholbert) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a6e53d96b83
Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/82b13a9a70e6
Part 2: Add a web-platform-test to check computed margin values on auto margin blocks. r=dholbert
Backed out 2 changesets (bug 1471894) for linting failure in /builds/worker/checkouts/gecko/tools/lint/wpt.yml on a CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=189671197
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189671197&repo=autoland&lineNumber=264

[task 2018-07-23T23:54:34.385Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-07-23T23:54:34.406Z] No specific files specified, running the full wpt lint (this is slow)
[task 2018-07-23T23:54:47.085Z]  0:12.68 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/mozilla/meta/MANIFEST.json
[task 2018-07-23T23:54:47.096Z]  0:12.69 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json
[task 2018-07-23T23:54:51.365Z]  0:16.96 WARNING Manifest /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json contains correct tests but file hashes changed; please update
[task 2018-07-23T23:58:05.277Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/lint/wpt.yml:None | Lint process exited with return code 1 (wpt)
[task 2018-07-23T23:58:05.277Z] TEST-UNEXPECTED-ERROR | testing/web-platform/tests/css/cssom/computed-style-005.html:None | Testcase file must have a link to a spec (MISSING-LINK)
[taskcluster 2018-07-23 23:58:05.618Z] === Task Finished ===
[taskcluster 2018-07-23 23:58:05.620Z] Unsuccessful task run with exit code: 1 completed in 458.24 seconds
Flags: needinfo?(bwerth)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f009f26663d
Backed out 2 changesets for linting failure in  /builds/worker/checkouts/gecko/tools/lint/wpt.yml on a CLOSED TREE
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12153 for changes under testing/web-platform/tests
Upstream PR was closed without merging
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/194afde62e69
Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/189749cc1e12
Part 2: Add a web-platform-test to check computed margin values on auto margin blocks. r=dholbert
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcda465b0522
Backed out 2 changesets for lint failure on wpt.yml CLOSED TREE
I'm sorry; I missed the lint log message that said that the test needed a link to the spec. I'll push a fix shortly.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7342b2da7568
Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a20576472778
Part 2: Add a web-platform-test to check computed margin values on auto margin blocks. r=dholbert
Sorry, the crashtests are from another push.
(In reply to Bogdan Tara[:bogdan_tara] from comment #49)
> Sorry, the crashtests are from another push.

I saw that; sorry I wasn't on IRC when you were looking for me. Does this mean that you can reland it? Please clarify if I need to make modifications or push again, etc.
There were similar wpt failures on your second land. Is everything ok?
If everything is ok, please reland it.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ab2fb716899
Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/bce3c6ca2e19
Part 2: Add a web-platform-test to check computed margin values on auto margin blocks. r=dholbert
(In reply to Bogdan Tara[:bogdan_tara] from comment #51)
> There were similar wpt failures on your second land. Is everything ok?
> If everything is ok, please reland it.

I didn't see those, ugh. It's possible my 4th push will bounce also. I'm very sorry. I'm doing a full wpt run now and I'll address any errors before attempting *another* landing.
Backed out 2 changesets (bug 1471894) for wpt failures on non-replaced-elements/flow-content-0/dialog.html.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bce3c6ca2e19a87381564a4a3cae0c0896482159

Backout link: https://hg.mozilla.org/integration/autoland/rev/e56525462fdbf43851d9e7df487babbfbaa18222

Failure log: ASAN example because this failed first https://treeherder.mozilla.org/logviewer.html#?job_id=189901611&repo=autoland&lineNumber=6064 
[task 2018-07-24T21:12:13.382Z] 21:12:13     INFO - TEST-START | /html/rendering/non-replaced-elements/flow-content-0/dialog.html
[task 2018-07-24T21:12:14.050Z] 21:12:14     INFO - PID 13754 | 1532466734037	Marionette	DEBUG	[2147483659] Frame script loaded
[task 2018-07-24T21:12:14.051Z] 21:12:14     INFO - PID 13754 | 1532466734038	Marionette	DEBUG	[2147483659] Frame script registered
[task 2018-07-24T21:12:14.370Z] 21:12:14     INFO - PID 13754 | JavaScript error: http://web-platform.test:8000/html/rendering/non-replaced-elements/flow-content-0/support/dialog-framed.html, line 12: TypeError: dialogModal.showModal is not a function
[task 2018-07-24T21:12:14.370Z] 21:12:14     INFO - PID 13754 | JavaScript error: http://web-platform.test:8000/html/rendering/non-replaced-elements/flow-content-0/support/dialog-framed.html, line 12: TypeError: dialogModal.showModal is not a function
[task 2018-07-24T21:12:14.455Z] 21:12:14     INFO - 
[task 2018-07-24T21:12:14.455Z] 21:12:14     INFO - TEST-FAIL | /html/rendering/non-replaced-elements/flow-content-0/dialog.html | Closed dialog in width: 540px iframe - assert_equals: width expected "fit-content" but got "-moz-fit-content"
[task 2018-07-24T21:12:14.455Z] 21:12:14     INFO - onload/<@http://web-platform.test:8000/html/rendering/non-replaced-elements/flow-content-0/dialog.html:47:7
[task 2018-07-24T21:12:14.456Z] 21:12:14     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
[task 2018-07-24T21:12:14.457Z] 21:12:14     INFO - test@http://web-platform.test:8000/resources/testharness.js:545:9
[task 2018-07-24T21:12:14.457Z] 21:12:14     INFO - onload@http://web-platform.test:8000/html/rendering/non-replaced-elements/flow-content-0/dialog.html:40:5
[task 2018-07-24T21:12:14.457Z] 21:12:14     INFO - EventHandlerNonNull*@http://web-platform.test:8000/html/rendering/non-replaced-elements/flow-content-0/dialog.html:34:1
[task 2018-07-24T21:12:14.458Z] 21:12:14     INFO - TEST-UNEXPECTED-PASS | /html/rendering/non-replaced-elements/flow-content-0/dialog.html | Open dialog in width: 540px iframe - expected FAIL
[task 2018-07-24T21:12:14.458Z] 21:12:14     INFO - TEST-INFO | expected FAIL
[task 2018-07-24T21:12:14.459Z] 21:12:14     INFO - 
[task 2018-07-24T21:12:14.459Z] 21:12:14     INFO - TEST-FAIL | /html/rendering/non-replaced-elements/flow-content-0/dialog.html | Modal dialog in width: 540px iframe - assert_equals: display expected "block" but got "none"
Attachment #8994696 - Flags: review?(dholbert)
Comment on attachment 8994696 [details]
Bug 1471894 Part 3: Update expectation to pass for parts of a WPT test.

https://reviewboard.mozilla.org/r/259224/#review266222
Attachment #8994696 - Flags: review?(dholbert) → review+
Upstream PR was closed without merging
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9a62b4744fe
Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/16989caa1055
Part 2: Add a web-platform-test to check computed margin values on auto margin blocks. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/62f4d1504b70
Part 3: Update expectation to pass for parts of a WPT test. r=dholbert
Upstream PR was closed without merging
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Flags: needinfo?(bwerth)
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1298008
[User impact if declined]: jQuery will give bad results to offset() calls on position:absolute elements with auto margins. The Website X5 Engine relies on this to implement their sticky menu bar widget. Website X5 Engine is moderately popular with smaller e-commerce sites.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Continues the work of Bug 1298008, handling an edge case. Bug 1298008 patch has been behaving well.
[String changes made/needed]: none
Attachment #8991486 - Flags: approval-mozilla-beta?
Comment on attachment 8991486 [details]
Bug 1471894 Part 1: Ensure that absolute positioned blocks with auto margins update their UsedMargin property.

chrome-parity, has been in Nightly for a few days, Beta62+
Attachment #8991486 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: