Closed Bug 1315660 Opened 3 years ago Closed 2 years ago

Image grows to double size when dragged

Categories

(Core :: Widget, defect, P2)

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: jsbr, Assigned: mb)

References

Details

(Keywords: regression, Whiteboard: tpi:+)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36 Edge/14.14393

Steps to reproduce:

Dragging certain draggable images in Firefox causes the dragged image to grow to double size. This behavior occurs in Firefox 49, but it did not occur in Firefox 48. The behavior also occurs in Firefox Beta and Development builds.

Reduced test case (works correctly in Chrome 54 and Firefox 48):
1. Open http://embed.plnkr.co/CGjU1iRIfw6BqNhyMwds/
2. Drag the border around the bottommost T icon as described on the test page


Actual results:

3. The ghosted T-icon with the border grows to double size


Expected results:

3. The ghosted T-icon with the border keeps its original size
User Agent of the Firefox browser I used to reproduce the bug in:
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
(Disregard the UA added to the bug description automatically)
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f8e784f16f69e9f5f6e37892c3057bc7d4fe0c86&tochange=1ef763f4dc5b833dacd5e3863b40c5e87ebeecbd

Bug 732733 - Resize drag image relative to the available screensize. r=jimm
Blocks: 732733
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Widget
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Priority: -- → P2
Whiteboard: tpi:+
Seems like we just need to make the thumbnail smaller when the image is smaller than the default thumbnail size (maybe?).
Moritz, would you be able to take a look at this regression?
Flags: needinfo?(moritzbrunner)
Too late for a fix for 53, as we are in the last week of the 53 beta cycle.
Pretty unlikely Moritz is going to respond at this point. Into the backlog it goes.
Welcome back!
Assignee: nobody → moritzbrunner
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

Jim reviewed the original patch and I think he's a more appropriate reviewer for this one too.
Attachment #8919827 - Flags: review?(ryanvm) → review?(jmathies)
I will note that there are a couple instances of tabs being used instead of spaces in that patch.
Formatting should now be fixed, thanks.
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review195966
Attachment #8919827 - Flags: review?(ryanvm) → review?(jmathies)
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review203426

update for nits and I think we're in good shape. I'll push to try for a test run before final sign off.

::: layout/base/PresShell.cpp:4984
(Diff revision 2)
> -      // half the difference and add it to worstHeight,
> -      // then get scalefactor to reach this
> +      // halve the difference and add it to worstHeight to get the best compromise between bestHeight and bestWidth,
> +      // then calculate the corresponding scale factor
>        scale = (worstHeight + difference / 2) / float(pixelArea.height);
> +      // prevent upscaling
> +      if (scale > 1)
> +        scale = 1;

nit - please wrap this in {}

::: widget/nsBaseDragService.cpp:672
(Diff revision 2)
>        uint32_t count = 0;
>        nsAutoString childNodeName;
>  
> -      if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList))) &&
> -          NS_SUCCEEDED(childList->GetLength(&length))) {
> -        // check every childnode for being a img-tag
> +      // check if the dragged node itself is an img element
> +      if (NS_SUCCEEDED(dragNode->GetNodeName(childNodeName))
> +          && childNodeName.LowerCaseEqualsLiteral("img")) {

nit - && on the end of the line above

::: widget/nsBaseDragService.cpp:679
(Diff revision 2)
> +      } else if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList)))
> +          && NS_SUCCEEDED(childList->GetLength(&length))) {
> +        // check every childnode for being an img element
>          while (count < length) {
> -          if (NS_FAILED(childList->Item(count, getter_AddRefs(child))) ||
> -              NS_FAILED(child->GetNodeName(childNodeName))) {
> +          if (NS_FAILED(childList->Item(count, getter_AddRefs(child)))
> +              || NS_FAILED(child->GetNodeName(childNodeName))) {

nit - why did you update the position of ||? lets remove the unneeded formatting changes here.
Attachment #8919827 - Flags: review?(jmathies) → review-
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

Stephen I don't have time for this this week or next. Can you try to find some time to review this? It related to drag images.
Attachment #8919827 - Flags: review?(jmathies) → review?(spohl.mozilla.bugs)
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review211028

Almost there. Thanks!

::: layout/base/PresShell.cpp:4979
(Diff revision 3)
>        scale = bestWidth / float(pixelArea.width);
>        // get the worst height (height when width is perfect)
>        float worstHeight = float(pixelArea.height)*scale;
>        // get the difference of best and worst height
>        float difference = bestHeight - worstHeight;
> -      // half the difference and add it to worstHeight,
> +      // halve the difference and add it to worstHeight to get the best compromise between bestHeight and bestWidth,

nit: over 80 characters

::: layout/base/PresShell.cpp:4983
(Diff revision 3)
>        float difference = bestHeight - worstHeight;
> -      // half the difference and add it to worstHeight,
> -      // then get scalefactor to reach this
> +      // halve the difference and add it to worstHeight to get the best compromise between bestHeight and bestWidth,
> +      // then calculate the corresponding scale factor
>        scale = (worstHeight + difference / 2) / float(pixelArea.height);
> +      // prevent upscaling
> +      if (scale > 1) {

Instead of assigning to |scale| directly inside this if block at line 4974 and line 4981, let's assign to a new local such as |float adjustedScale|. Then, use |scale = std::min(scale, adjustedScale);| here to prevent upscaling.

::: layout/base/PresShell.cpp:4991
(Diff revision 3)
>      } else {
>        // get half of max screensize
>        nscoord maxWidth = pc->AppUnitsToDevPixels(maxSize.width >> 1);
>        nscoord maxHeight = pc->AppUnitsToDevPixels(maxSize.height >> 1);
>        if (pixelArea.width > maxWidth || pixelArea.height > maxHeight) {
>          scale = 1.0;

nit: this isn't technically needed here since |scale| is still 1.0 from its initialization above.

::: widget/nsBaseDragService.cpp:674
(Diff revision 3)
>  
> -      if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList))) &&
> -          NS_SUCCEEDED(childList->GetLength(&length))) {
> -        // check every childnode for being a img-tag
> +      // check if the dragged node itself is an img element
> +      if (NS_SUCCEEDED(dragNode->GetNodeName(childNodeName)) &&
> +          childNodeName.LowerCaseEqualsLiteral("img")) {
> +        renderFlags = renderFlags | nsIPresShell::RENDER_IS_IMAGE;
> +      } else if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList)))

nit: over 80 characters

::: widget/nsBaseDragService.cpp:675
(Diff revision 3)
> -      if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList))) &&
> -          NS_SUCCEEDED(childList->GetLength(&length))) {
> -        // check every childnode for being a img-tag
> +      // check if the dragged node itself is an img element
> +      if (NS_SUCCEEDED(dragNode->GetNodeName(childNodeName)) &&
> +          childNodeName.LowerCaseEqualsLiteral("img")) {
> +        renderFlags = renderFlags | nsIPresShell::RENDER_IS_IMAGE;
> +      } else if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList)))
> +          && NS_SUCCEEDED(childList->GetLength(&length))) {

nit: && should go at end of previous line
nit: fix indent to align NS_SUCCEEDED with NS_SUCCEEDED on the previous line.
Attachment #8919827 - Flags: review?(spohl.mozilla.bugs) → review-
Attachment #8919827 - Flags: review?(jmathies)
Attachment #8919827 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review212988

A lot of seemingly unrelated changes made it into this last patch. Could you reduce it back to the original changes? Thanks.
Attachment #8919827 - Flags: review?(spohl.mozilla.bugs) → review-
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review212988

Sorry, added them by accident.
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review213014

Just one nit left to address. Thank you!

::: layout/base/PresShell.cpp:4975
(Diff revisions 3 - 5)
>        // get best height/width relative to screensize
>        float bestHeight = float(maxHeight)*RELATIVE_SCALEFACTOR;
>        float bestWidth = float(maxWidth)*RELATIVE_SCALEFACTOR;
> +      float adjustedScale;
>        // calculate scale for bestWidth
> -      scale = bestWidth / float(pixelArea.width);
> +      adjustedScale = bestWidth / float(pixelArea.width);

nit: declare and assign |float adjustedScale| on this same line here.
Attachment #8919827 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review213018


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: layout/base/PresShell.cpp:4975
(Diff revision 5)
>        // get best height/width relative to screensize
>        float bestHeight = float(maxHeight)*RELATIVE_SCALEFACTOR;
>        float bestWidth = float(maxWidth)*RELATIVE_SCALEFACTOR;
> -      // get scalefactor to reach bestWidth
> -      scale = bestWidth / float(pixelArea.width);
> +      float adjustedScale;
> +      // calculate scale for bestWidth
> +      adjustedScale = bestWidth / float(pixelArea.width);

Warning: Value stored to 'adjustedscale' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review213020

::: layout/base/PresShell.cpp:4983
(Diff revision 5)
>        // get the difference of best and worst height
>        float difference = bestHeight - worstHeight;
> -      // half the difference and add it to worstHeight,
> -      // then get scalefactor to reach this
> -      scale = (worstHeight + difference / 2) / float(pixelArea.height);
> +      // halve the difference and add it to worstHeight to get
> +      // the best compromise between bestHeight and bestWidth,
> +      // then calculate the corresponding scale factor
> +      adjustedScale = (worstHeight + difference / 2) / float(pixelArea.height);

Static analysis just made it obvious that we never read the value of adjustedScale found on line 4975, so we can just remove the code on line 4975, then declare and assign |float adjustedScale| on this line here instead.
Comment on attachment 8919827 [details]
Bug 1315660 - Fixed upscaling of drag and drop thumbnail.

https://reviewboard.mozilla.org/r/190748/#review213020

> Static analysis just made it obvious that we never read the value of adjustedScale found on line 4975, so we can just remove the code on line 4975, then declare and assign |float adjustedScale| on this line here instead.

Actually I forgot to rename scale to adjustedScale.
(In reply to Moritz Brunner [:mb] from comment #26)
> Actually I forgot to rename scale to adjustedScale.

Phew, glad you caught that. LGTM. Thanks!
Pushed by spohl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46d47762f633
Fixed upscaling of drag and drop thumbnail. r=spohl
https://hg.mozilla.org/mozilla-central/rev/46d47762f633
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Too late for Beta58. Mark 58 won't fix.
You need to log in before you can comment on or make changes to this bug.