Last Comment Bug 717143 - Remove bogus margins when force-reloading standalone image
: Remove bogus margins when force-reloading standalone image
Product: Core
Classification: Components
Component: Layout: Images (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla13
Assigned To: Frank Yan (:fryn)
: Jet Villegas (:jet)
Depends on:
Blocks: 726402
  Show dependency treegraph
Reported: 2012-01-10 19:49 PST by :Felipe Gomes (needinfo me!)
Modified: 2013-11-13 02:20 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.73 KB, patch)
2012-02-10 15:51 PST, Frank Yan (:fryn)
bzbarsky: review+
felipc: feedback+
Details | Diff | Splinter Review

Description User image :Felipe Gomes (needinfo me!) 2012-01-10 19:49:40 PST
When viewing a standalone image that was shrunk to fit the screen, a force-reload of the page reloads the image but adds margins to it.

- open an image that is larger than the window. e.g.
- notice that the image is connected to the top/bottom edge of the content area
- Press Ctrl+Shift+R
- notice that now there's an 8px margin on the image

Any resizing of the window brings it back to normal. DOM Inspector tells everything has 0 on margins, but displays 8px for the top/bottom computed style of the <img>, despite being nothing in the stylesheets setting that.
Comment 1 User image Frank Yan (:fryn) 2012-02-10 15:51:25 PST
Created attachment 596202 [details] [diff] [review]

The problem is that CheckOverflowing() is deflating the visibleArea by the sizes of the <body/>'s margins, borders, and padding, but the values that it gets for the margin are erroneously non-zero when force-reloading an image, causing this bug. We explicitly apply zero margins to the <body/> for ImageDocuments anyway, so I removed that unnecessary code.

Do we still need the extra block (scope) and the comment at the top of it?
Comment 2 User image :Felipe Gomes (needinfo me!) 2012-02-10 16:18:00 PST
Comment on attachment 596202 [details] [diff] [review]

This fixes the problem.

It's weird that mCachedMargins has the wrong default margins after a force-refresh, but I guess it's expected as the stylesheet hasn't been loaded at that point.

anyway, it seems like removing code that is doing unecessary work and producing the wrong results is a good compromise!
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-17 22:05:44 PST
Comment on attachment 596202 [details] [diff] [review]

Comment 4 User image Mozilla RelEng Bot 2012-02-17 23:26:39 PST
Autoland Patchset:
	Patches: 596202
	Branch: mozilla-central => try
Try run started, revision 7425b6a8bae5. To cancel or monitor the job, see:
Comment 5 User image Mozilla RelEng Bot 2012-02-18 03:31:06 PST
Try run for 7425b6a8bae5 is complete.
Detailed breakdown of the results available here:
Results (out of 212 total builds):
    success: 166
    warnings: 32
    failure: 14
Builds (or logs if builds failed) available at:
Comment 6 User image Frank Yan (:fryn) 2012-02-18 03:41:42 PST
We need to land this at the same time as my fix for bug 726402 (a random orange related to this), so wait for that.
Comment 7 User image Frank Yan (:fryn) 2012-03-07 22:28:19 PST
Thanks for the review.
Comment 8 User image Rob Campbell [:rc] (:robcee) 2012-03-08 06:48:27 PST

Note You need to log in before you can comment on or make changes to this bug.