Closed Bug 1315846 Opened 4 years ago Closed 4 years ago

Intermittent reftests/details-summary/mouse-click-overflow-auto-details.html == reftest/tests/layout/reftests/details-summary/overflow-auto-open-details.html | image comparison, max difference: 1, number of differing pixels: 1674

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: neerja, Assigned: neerja)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

No description provided.
Assignee: nobody → npancholi
Keywords: leave-open
(I removed "leave-open" because the patch here is aiming to rewrite the reftest such that it won't intermittently fail at all anymore. This is semantically different from other intermittent-orange bugs where you're adding "fuzzy" annotations -- in those cases, the test technically still fails but we just ignore the failure, so "leave-open" is appropriate because the test is still technically failing. It's debatable whether it's worth leaving those other bugs open, but we definitely don't need to bother leaving this one open after the patch has landed.)
Comment on attachment 8808823 [details]
Bug 1315846 - Fix reftests with minor AA and layerization issues by replacing text with tall divs as placeholders

https://reviewboard.mozilla.org/r/91554/#review91398

Nearly there! Two minor nits, which apply to all three files that you're modifying here:

::: layout/reftests/details-summary/mouse-click-overflow-auto-details.html:26
(Diff revision 1)
>      height: 100px;
>    }
> +  div.tall {
> +    background-color: blue;
> +    border: 1px dotted purple;
> +    height: 1000px;

Please add "width: 50px;" on the line before "height: 1000px".

(This makes it a little easier to distinguish the tall child from its container, which makes it easier to diagnose what's going wrong if/when this test ever starts failing in the future.)

::: layout/reftests/details-summary/mouse-click-overflow-auto-details.html:41
(Diff revision 1)
>        </summary>
> -      Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod
> -      tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
> +        <div class="tall">
> +        </div>

Nit: please deindent your second added <div> here, so that it's at the same level of indentation as its sibling (the <summary> element).
Attachment #8808823 - Flags: review?(dholbert) → review-
Comment on attachment 8808823 [details]
Bug 1315846 - Fix reftests with minor AA and layerization issues by replacing text with tall divs as placeholders

https://reviewboard.mozilla.org/r/91554/#review91410

Looks good! r=me
Attachment #8808823 - Flags: review?(dholbert) → review+
Comment on attachment 8808823 [details]
Bug 1315846 - Fix reftests with minor AA and layerization issues by replacing text with tall divs as placeholders

https://reviewboard.mozilla.org/r/91554/#review91410

Thanks Daniel! :)
Sure! Looks like this was part of a clean Try run, here:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=0268d8e0c8def2152d1b9adb390c7687e8b4608c
so this is ready to land! --> triggering autoland.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/151c40efcb3c
Fix reftests with minor AA and layerization issues by replacing text with tall divs as placeholders r=dholbert
https://hg.mozilla.org/mozilla-central/rev/151c40efcb3c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.