Closed Bug 1078337 Opened 5 years ago Closed 5 years ago

Line next to find button in menu

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 --- fixed

People

(Reporter: wesj, Assigned: mattwoodrow)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached image find.png
Theres a tiny thin line next to the find button.
If this regressed recently, it's probably due to bug 1044702.
I can reproduce this, and yes, I'm fairly sure this is a recent regression.
Component: Theme → Untriaged
Product: Firefox → Core
Version: unspecified → Trunk
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a61ddc0c62fd&tochange=6fb508e952b1

6fb508e952b1	Matt Woodrow — Bug 1070722 - Use the imagelib high quality downscaler on OSX instead of the quartz one. r=jrmuizel
[Tracking Requested - why for this release]: regression
I had a chat with seth about this.

It looks like we're using a spritesheet for these images, and the xul image box has crazy subpixel positioning and sizing.

The combination of these things mean that we end up resizing the image slightly, and sampling from some of the pixels of the next image in the sheet, leaving these grey lines.

The best solution is probably just to change the spritesheet to have slightly larger gaps between images.
Component: Untriaged → Theme
Product: Core → Firefox
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> I had a chat with seth about this.
> 
> It looks like we're using a spritesheet for these images, and the xul image
> box has crazy subpixel positioning and sizing.
> 
> The combination of these things mean that we end up resizing the image
> slightly, and sampling from some of the pixels of the next image in the
> sheet, leaving these grey lines.
> 
> The best solution is probably just to change the spritesheet to have
> slightly larger gaps between images.

That doesn't sound right at all. We should be able to specify exactly the area we need. Are you saying we're doing this incorrectly? Or is Gecko not doing what it's being told?
Flags: needinfo?(seth)
Flags: needinfo?(matt.woodrow)
Concurring with Dão, we should be able to specify the region and have it "just work". It's not clear to me why the positioning or sizing of the box even matters - we specify the image file and a source region in that image file, and that's what it should use for rendering the background, nothing outside that region (irrespective of the size, antialiasing or otherwise of the box that we happen to use the background image on).

I'd also note that updating all our sprites across platforms and updating all the image regions in the CSS for those sprites would be a considerable task, likely with fallout and regressions, that I wouldn't really want to have to land with less than a week to go until uplift...
Ah, I didn't realize that the xul imagebox element let you explicitly specify a subrect to use. That's nice, and makes it more likely a gecko bug.
Component: Theme → ImageLib
Flags: needinfo?(matt.woodrow)
Product: Firefox → Core
Attached patch Use the source rect (obsolete) — Splinter Review
Looks like we basically entirely ignore the source rect that xul is specifying. We use it to create an ClippedImage, and then don't use that.

This seems to work, what do you think seth?
Assignee: nobody → matt.woodrow
Attachment #8502361 - Flags: review?(seth)
seth, I also notice that ClippedImage won't take advantage of source clipping through Moz2D. Is there a plan to change that?
Flags: needinfo?(seth)
Comment on attachment 8502361 [details] [diff] [review]
Use the source rect

Review of attachment 8502361 [details] [diff] [review]:
-----------------------------------------------------------------

Oh wow, facepalm. Thanks for tracking this down.
Attachment #8502361 - Flags: review?(seth) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> seth, I also notice that ClippedImage won't take advantage of source
> clipping through Moz2D. Is there a plan to change that?

We definitely should change that. I have some questions before I make the change, so I'll hold off on filing the bug until we can talk and I know exactly what bug to file.
Backed out for reftest failure and mochitest-bc crashes. Please make sure this is green on Try before pushing again. Especially given that this is the weekend before the uplift and tree closures and bustage can be rather disruptive.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f547cf19d104

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2948117&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2947705&repo=mozilla-inbound
Attached patch Use the source rect v2 (obsolete) — Splinter Review
This version passes try.
Attachment #8504472 - Flags: review?(seth)
Attachment #8502361 - Attachment is obsolete: true
Comment on attachment 8504472 [details] [diff] [review]
Use the source rect v2

Review of attachment 8504472 [details] [diff] [review]:
-----------------------------------------------------------------

This approach is an option if we can't do better, but I have the feeling this can be made cleaner.

Specifically...

::: layout/base/nsLayoutUtils.h
@@ +1599,5 @@
> +   * aImageSourceArea and the original image size.
> +   */
> +  static nsRect GetClippedImageDestination(const nsIntSize& aWholeImageSize,
> +                                           const nsRect& aImageSourceArea,
> +                                           const nsRect& aDestArea);

I'd prefer not to add this. Can't we get the same effect by munging |source| and |imageSize| in |DrawSingleImage|? Both of those variables only exist to serve as arguments for |GetWholeImageDestination|, so it's fine to adjust them as needed to make |GetWholeImageDestination| happy.
Attachment #8504472 - Flags: review?(seth) → review-
Ok, how about this then?
Attachment #8504472 - Attachment is obsolete: true
Attachment #8508475 - Flags: review?(seth)
Comment on attachment 8508475 [details] [diff] [review]
Use the source rect v3

Review of attachment 8508475 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good!

::: layout/base/nsLayoutUtils.cpp
@@ +5328,5 @@
> +                   imageSize.width*appUnitsPerCSSPixel,
> +                   imageSize.height*appUnitsPerCSSPixel);
> +  nsRect clippedSource = imageRect.Intersect(source);
> +
> +  source -= clippedSource.TopLeft();

You only need to do this clipping if |aSourceArea| is non-null, so you could move this into the |if (aSourceArea)| block and change the value of |imageSize| directly. The common case is that |aSourceArea| is null (I think only XUL uses this parameter?) so it might be nice to move this work out of the main path.
Attachment #8508475 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/f831a50b8976
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Duplicate of this bug: 1088301
I think this should be uplifted to aurora because of bug 1077997
Matt, can this be uplifted to aurora please? See comment 23
Flags: needinfo?(matt.woodrow)
Comment on attachment 8508475 [details] [diff] [review]
Use the source rect v3

Approval Request Comment
[Feature/regressing bug #]: Bug 1044702.
[User impact if declined]: Incorrect clipping of some XUL images, visible lines shown at edges.
[Describe test coverage new/current, TBPL]: Tested manually.
[Risks and why]: Low risk, fairly simple patch in the end, only affects XUL.
[String/UUID change made/needed]: None.
Flags: needinfo?(matt.woodrow)
Attachment #8508475 - Flags: approval-mozilla-aurora?
Attachment #8508475 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.