Closed
Bug 1326910
Opened 4 years ago
Closed 4 years ago
"Page Info" displays wrong image dimensions the 1st time (0px ? 0px)
Categories
(Firefox :: Page Info Window, defect)
Firefox
Page Info Window
Tracking
()
VERIFIED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: arni2033, Assigned: junderx)
Details
Attachments
(2 files, 7 obsolete files)
8.45 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
>>> My Info: Win7_64, Nightly 49, 32bit, ID 20160526082509 STR_1: 1. Open bug 1257815 2. Right-click on the 1st user avatar on the page, click "View Image Info" 3. Close "Page Info" window, repeat Step 2 AR: Step 2 - "Page Info" window says image dimensions are "0px ? 0px (scaled to 32px ? 32px)" Step 3 - "Page Info" window says image dimensions are "64px ? 64px (scaled to 32px ? 32px)" ER: Either X or Y X) Step 2 - "Page Info" should display the correct data, i.e. "64px ? 64px (scaled to 32px ? 32px)" Y) Firefox UI should underline that it's not reliable, and ask user to switch to repeat Step 2
The root cause of the bug was that the width and height of the newImage instance were being used before the instance was finished loading from the url. The proposed fix sets up a loadend event which calls a new function after the event that accesses the width and height of the loaded image.
Attachment #8832379 -
Flags: review?(florian)
Comment 2•4 years ago
|
||
Comment on attachment 8832379 [details] [diff] [review] 1326910.patch - Proposed fix for Bug 1326910 Review of attachment 8832379 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking into this. The approach in the patch seems good to me, I agree we need to wait for the load to finish before getting the size. ::: browser/base/content/pageinfo/pageInfo.js @@ +843,5 @@ > newImage.id = "thepreviewimage"; > var physWidth = 0, physHeight = 0; > var width = 0, height = 0; > > + // The item instance is already loaded before we get here I don't understand the meaning of this comment, could you please explain or rephrase? @@ +850,5 @@ > (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) || > isBG) && isProtocolAllowed) { > newImage.setAttribute("src", url); > + // We need to wait for the image to finish loading before using width & height > + newImage.addEventListener("loadend", imgSize, false); - It would be safer to set the listener before setting the "src" attribute. - is a removeEventListener call missing? Or maybe you want to use the {once: true} option? - nit: please omit the optional third parameter of addEventListener when it's 'false'. @@ +889,5 @@ > + document.getElementById("theimagecontainer").collapsed = false > + document.getElementById("brokenimagecontainer").collapsed = true; > + > + let imageSize = ""; > + if (url && !isAudio) { I know you just moved this code, but we don't need this !isAudio check here, right? @@ +907,5 @@ > + } > + } > + > + // Handle the case where newImage is not used for width & height > + if (!((item.HTMLLinkElement || item.HTMLInputElement || Isn't this just "else if (..." like before? I don't see the reason why you need to change this.
Attachment #8832379 -
Flags: review?(florian) → feedback+
Patch with all the nice change recommendations implemented
Attachment #8832379 -
Attachment is obsolete: true
Attachment #8832784 -
Flags: review?(florian)
Comment 4•4 years ago
|
||
Comment on attachment 8832784 [details] [diff] [review] Patch that includes recommended changes Review of attachment 8832784 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating the patch! I tried the patch and it worked fine for me. I will likely r+ the next version. ::: browser/base/content/pageinfo/pageInfo.js @@ +848,5 @@ > item.HTMLImageElement || item.SVGImageElement || > (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) || > isBG) && isProtocolAllowed) { > + // We need to wait for the image to finish loading before using width & height > + newImage.addEventListener("loadend", imgSize, {once: true}); I'm not really comfortable with this using a function that's only declared a few lines later. Given that this function is used only once, I would inline it: newImage.addEventListener("loadend", function() { ... }, {once: true}); newImage.setAttribute("src", ... I should have mentioned this during the previous review, sorry :-/.
Attachment #8832784 -
Flags: review?(florian) → feedback+
Changed the imgSize() function to inline since it is only called one place.
Attachment #8832784 -
Attachment is obsolete: true
Attachment #8833193 -
Flags: review?(florian)
Comment 6•4 years ago
|
||
Comment on attachment 8833193 [details] [diff] [review] Use inline function this time. Looks good to me, thanks! Your patch doesn't include a header with the author and commit message, which name would you like us to use to credit you when checking in the patch?
Flags: needinfo?(junderx)
Attachment #8833193 -
Flags: review?(florian) → review+
Hi Florian, Please use my full name, John Underwood. You could also mention junderx. Thanks, John U
Comment 8•4 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8475919c50b5e7874af3856ae8a8cc4778939102 Bug 1326910 - wait for a loadend event before using the width and height of the Page Info media preview, r=florian.
Comment 9•4 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=74764752&repo=mozilla-inbound
Comment 10•4 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/40ea52619427 Backed out changeset 8475919c50b5 for test failures in browser_bug517902.js on a CLOSED TREE
Comment 11•4 years ago
|
||
Usually I would have pushed to the try server a patch changing the browser UI, to verify that it passes all the tests before checking it in, but I thought this code wasn't covered by tests. Turns out I was wrong and there's http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug517902.js The next step here will be to figure out if the test failure is caused by a bug in this test, or is showing a real problem with the patch.
Assignee | ||
Comment 12•4 years ago
|
||
I'll take a look to sort it out.
Assignee | ||
Comment 13•4 years ago
|
||
The http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug517902.js test is checking the size of the about:logo image. It says that the width is expected to be 150 and the height to be 100. I reverted the pageInfo.js source file and ran mach build again. Then I loaded the about:logo page after starting the build result and selected View Image Info while stopping at break points in the debugger to check width and height variables. No variations of the width and height variables ever had anything except 0 or 300 for width and 0 or 236 for height. So my conclusion is that the browser_bug517902.js test should fail even with this bug fix reverted.
Comment 14•4 years ago
|
||
(In reply to junderx from comment #13) > The > http://searchfox.org/mozilla-central/source/browser/base/content/test/ > general/browser_bug517902.js test is checking the size of the about:logo > image. It's checking the width and height of the preview image, which previews <img src='about:logo?b' height=100 width=150 alt=2 id='test-image'>. So expecting 100 and 150 is reasonable. What's happening here is that when your patch is applied, the width and height attributes are set on the preview image only after it has finished loading. The test needs to be changed to wait for the loadend event on the preview image before checking the width and height (and I think we can remove the 'executeSoon' call currently in the test).
Assignee | ||
Comment 15•4 years ago
|
||
Changing the test to use a callback for testImg loaded event did not work. The callback was never called. This may have been due to the fact that the pageinfo.js code was already using that event and callback. This patch just waits 1 second for the pageinfo.js code to finish before testing the result. I tried 300ms and that also works, but 30ms did not work.
Attachment #8839261 -
Flags: review?(florian)
Comment 16•4 years ago
|
||
Comment on attachment 8839261 [details] [diff] [review] Patch for the test that was failing (In reply to junderx from comment #15) > Changing the test to use a callback for testImg loaded event did not work. > The callback was never called. We'll need to figure this out. Can you show me the code you tried and that didn't work? > This patch just > waits 1 second for the pageinfo.js code to finish before testing the result. > I tried 300ms and that also works, but 30ms did not work. We can't just use a timer here, or it'll fail intermittently on all machines that are slower than yours (or under heavy load).
Attachment #8839261 -
Flags: review?(florian) → review-
Assignee | ||
Comment 17•4 years ago
|
||
Attachment #8843637 -
Flags: review?(florian)
Assignee | ||
Comment 18•4 years ago
|
||
Assignee | ||
Comment 19•4 years ago
|
||
Assignee | ||
Comment 20•4 years ago
|
||
Assignee | ||
Comment 21•4 years ago
|
||
I have attached 4 versions of the test that never reach the callback. Since this is test code and not the browser itself, I'm not sure we will ever be running on a slow or heavily loaded machine, but I suppose that is still a possibility.
Comment 22•4 years ago
|
||
Comment on attachment 8843637 [details]
First version of the test that fails to reach the callback
Just replace the executeSoon call by:
pageInfoImg.addEventListener("loadend", function() {
(and that means you need to move the "var pageInfoImg = ..." line up a little bit of course)
Attachment #8843637 -
Flags: review?(florian) → review-
Comment 23•4 years ago
|
||
(In reply to junderx from comment #21) > Since this is test code and not the browser itself, I'm not sure we will > ever be running on a slow or heavily loaded machine, but I suppose that is > still a possibility. You would be surprised; it's common, eg. when tests run on virtual machines.
Updated•4 years ago
|
Assignee: nobody → junderx
Assignee | ||
Comment 24•4 years ago
|
||
Looks like this completes this bug fix. The test passes now. If there is anything else that needs to be done for this, anyone please feel free to do it. I'm giving up my cloud server tonight on AWS.
Attachment #8839261 -
Attachment is obsolete: true
Attachment #8843637 -
Attachment is obsolete: true
Attachment #8843638 -
Attachment is obsolete: true
Attachment #8843639 -
Attachment is obsolete: true
Attachment #8843640 -
Attachment is obsolete: true
Flags: needinfo?(junderx)
Attachment #8865316 -
Flags: review?(florian)
Comment 25•4 years ago
|
||
Comment on attachment 8865316 [details] [diff] [review] Patch for the test that was failing Review of attachment 8865316 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! (and sorry for the long delay) The code change to the test looks good, but the indent shouldn't use tabs. Note: this test has been renamed to browser/base/content/test/pageinfo/browser_pageinfo_image_info.js
Attachment #8865316 -
Flags: review?(florian) → review+
Comment 26•4 years ago
|
||
Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aacc0f1690 wait for a loadend event before using the width and height of the Page Info media preview, r=florian.
![]() |
||
Comment 27•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e7aacc0f1690
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 28•4 years ago
|
||
I have reproduced this Bug on Nightly 53.0a1 (2017-01-01) on Windows 10, 64 Bit! The bug's fix is now verified on latest Nightly 55.0a1 Build ID 20170604030205 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 [bugday-20170607]
Updated•4 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•