613696 - Minefield button randomly changes sizes.

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Benjamin Xiao]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101119 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101119 Firefox/4.0b8pre

Sometimes the Minefield button changes size on hover. This happens the most when the screen resolution is changed (etc. while gaming). When the problem occurs, you first see that the Minefield text becomes more narrow, all the letters are squeezed together slightly. When you hover over the button, it changes size horizontally to accomodate the narrower text.

Reproducible: Sometimes

Steps to Reproduce:
1. Open Minefield
2. Change resolution (you might need to do this multiple times before the problem occurs)
3. See that the letters in "Minefield" is squeezed together slightly.
4. Hover over the button and watch it change in size
Actual Results:  
Minefield button changes horizontal size randomly.

Expected Results:  
Minefield button does not change in size.

Comment 1

[Christian Ascheberg]

Attachment: screenshot

I can confirm this on Win Vista. The labeling of the tabs is also affected. I am quite sure that this is a regression. I do not have a reliable STR but it is not necessary to change the resolution, it can also happen after opening / closing tabs or minimizing firefox.

Comment 2

[Benjamin Xiao]

Yes it is not necessary to change the resolution. It happens randomly, but I find the most reliable way to reproduce the bug is by switching resolutions. Otherwise the only other way is just to wait :(

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

I've been seeing this, as has :davida, we were both waiting for reliable STR.

Seems to be related to the font size in the button. On hover, it changes. Once it's changed, it stays that way for a while, then goes back?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Wondering if this might be related to bug 588880?

Comment 5

[Asa Dotzler [:asa]]

I suspect this does have to do with hardware accel.  I don't ever see this on my laptop which was blacklisted from all hardware acceleration but I have seen it on my wife's machine which isn't blacklisted.

Comment 6

[Joe Drew (not getting mail)]

I would be - is there a stronger word than flabbergasted? - struck deaf, dumb and blind by sheer astonishment if this had anything to do with hardware acceleration.

However, it almost certainly has to do with DirectWrite, which is often conflated with hardware acceleration. Can someone confirm that for me?

Comment 7

[David Ascher (:davida)]

@joe: how can we help confirm that?

Comment 8

[Asa Dotzler [:asa]]

You can disable direct write in about:config and see if the problem persists. 

Joe, you're right. I was conflating the two. DirectWrite, not HW Accel.

Comment 9

[Nochum Sossonko [:Natch]]

I wasn't able to find a way to disable DirectWrite via about:config. This definitely happens to me. This bug seems like it belongs in a Core components. I'm on Vista...

Mozilla/5.0 (Windows NT 6.0; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

gfx.font_rendering.directwrite.enabled

Comment 11

[Nochum Sossonko [:Natch]]

(In reply to comment #10)
> gfx.font_rendering.directwrite.enabled

Doesn't work. It's set to "disabled" but directwrite is still enabled (as seen on about:support).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You'll also have to ensure that D2D is not force-enabled, and that D3D9 layers are being used instead of D3D10. D3D10 requires D2D which requires DirectWrite...

Comment 13

[Nochum Sossonko [:Natch]]

Ok, so essentially the pref to change is gfx.direct2d.disabled. I am testing with this now.

Comment 14

[Christian Ascheberg]

I did a bit of regression testing (with no actual STR though):
works (?):
win32 2010-11-14-04-mozilla-central 674f2ed15cea
broken:
win32 2010-11-15-04-mozilla-central edf41ff32f08

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=674f2ed15cea&tochange=edf41ff32f08

(Though I've got the feeling that the bug happens more often in current builds.)

Comment 15

[Eamon Nerbonne]

Attachment: A clearer screenshot of several states stacked over each other.

Although the button just jumps a few pixels, it's more distracting than it sounds because it occurs exactly when your mouse is over it and your attention on the button.

Comment 16

[Krisztian Paczari]

There is a STR in bug 616384.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

There's a lot of good information in bug 616384, actually.

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Screen capture of the button changing size on hover

I'm seeing this bug pretty constantly throughout the day.  Here's a quick screen capture of the issue.

Comment 20

[Krisztian Paczari]

Attachment: AutoIt3 script reproducing the bug

I attached an AutoIt3 script which reproduces the issue. It can run forever and one can see the button changing back and forth. (This is much simpler than the STR in bug 616384.)

To use it just download and install AutoIt3: http://www.autoitscript.com/autoit3/downloads.shtml, then load the script in the SciTE Script Editor, then press Tools|Go.

Comment 21

[Jonathan Kew [:jfkthame]]

I've been looking into this a bit, and it appears that the text of the "Minefield" button is sometimes rendered to a D2D surface, and sometimes to an image surface. When it is rendered to a D2D surface, the cairo scaled_font has its hint_metrics flag set to CAIRO_HINT_METRICS_DEFAULT, but when it is rendered to an image surface, the hint_metrics flag is set to CAIRO_HINT_METRICS_ON. This results in differing glyph metrics, because when CAIRO_HINT_METRICS_ON is used, the harfbuzz shaper code rounds the "ideal" metrics it computes from the raw font data.

The D2D-surface call seems to occur when the textrun is constructed during reflow, presumably for the purpose of measuring the text, while the image-surface case occurs when the textrun is being (re)constructed during layer rendering. Presumably this can happen if the old textrun has expired from the cache, and then the next operation that needs it is a layer-drawing operation.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds right.

But in all cases I would have thought the context involved would be the result of PresShell::GetReferenceRenderingContext(), and the surface would be gfxPlatform::ScreenReferenceSurface(). When is the surface you get not gfxPlatform::ScreenReferenceSurface()?

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to comment #22)
> But in all cases I would have thought the context involved would be the result
> of PresShell::GetReferenceRenderingContext(), and the surface would be
> gfxPlatform::ScreenReferenceSurface().

This is true for the initial construction of the "Minefield" textrun. In this case, the surface is a D2D surface.

> When is the surface you get not
> gfxPlatform::ScreenReferenceSurface()?

Here's a stack for the case where the textrun is being rebuild during layer painting, and a context with an image surface has apparently been passed down from the layers code:

>xul.dll!gfxFont::InitTextRun(gfxContext * aContext=0x0fca4a98, gfxTextRun * aTextRun=0x0fc9b048, const wchar_t * aString=0x0030a7c4, unsigned int aRunStart=0, unsigned int aRunLength=10, int aRunScript=25, int aPreferPlatformShaping=0)  Line 1449C++
 xul.dll!gfxFontGroup::InitTextRun(gfxContext * aContext=0x0fca4a98, gfxTextRun * aTextRun=0x0fc9b048, const wchar_t * aString=0x0030a7c4, unsigned int aTotalLength=10, unsigned int aScriptRunStart=0, unsigned int aScriptRunEnd=10, int aRunScript=25)  Line 2427 + 0x27 bytesC++
 xul.dll!gfxFontGroup::InitTextRun(gfxContext * aContext=0x0fca4a98, gfxTextRun * aTextRun=0x0fc9b048, const wchar_t * aString=0x0030a7c4, unsigned int aLength=10)  Line 2391C++
 xul.dll!gfxFontGroup::MakeTextRun(const wchar_t * aString=0x0030a7c4, unsigned int aLength=10, const gfxTextRunFactory::Parameters * aParams=0x0030ad50, unsigned int aFlags=1)  Line 2367C++
 xul.dll!TextRunWordCache::MakeTextRun(const wchar_t * aText=0x094f82b8, unsigned int aLength=9, gfxFontGroup * aFontGroup=0x093c9ec0, const gfxTextRunFactory::Parameters * aParams=0x0030adac, unsigned int aFlags=0)  Line 726 + 0x30 bytesC++
 xul.dll!gfxTextRunWordCache::MakeTextRun(const wchar_t * aText=0x094f82b8, unsigned int aLength=9, gfxFontGroup * aFontGroup=0x093c9ec0, const gfxTextRunFactory::Parameters * aParams=0x0030adac, unsigned int aFlags=0)  Line 1044C++
 xul.dll!gfxTextRunCache::MakeTextRun(const wchar_t * aText=0x094f82b8, unsigned int aLength=9, gfxFontGroup * aFontGroup=0x093c9ec0, gfxContext * aRefContext=0x0fca4a98, unsigned int aAppUnitsPerDevUnit=60, unsigned int aFlags=0)  Line 93 + 0x19 bytesC++
 xul.dll!nsThebesFontMetrics::AutoTextRun::AutoTextRun(nsThebesFontMetrics * aMetrics=0x0af2ea78, nsIRenderingContext * aRC=0x0fcd8480, const wchar_t * aString=0x094f82b8, int aLength=9)  Line 171 + 0x3e bytesC++
 xul.dll!nsThebesFontMetrics::DrawString(const wchar_t * aString=0x094f82b8, unsigned int aLength=9, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000, nsThebesRenderingContext * aContext=0x0fcd8480)  Line 419C++
 xul.dll!nsThebesRenderingContext::DrawStringInternal(const wchar_t * aString=0x094f82b8, unsigned int aLength=9, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000)  Line 1388C++
 xul.dll!nsThebesRenderingContext::DrawString(const wchar_t * aString=0x094f82b8, unsigned int aLength=9, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000)  Line 1210 + 0x20 bytesC++
 xul.dll!nsThebesRenderingContext::DrawString(const nsString & aString={...}, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000)  Line 878C++
 xul.dll!nsTextBoxFrame::DrawText(nsIRenderingContext & aRenderingContext={...}, const nsRect & aTextRect={...}, const unsigned int * aOverrideColor=0x0030b030)  Line 554C++
 xul.dll!nsTextBoxFrame::PaintOneShadow(gfxContext * aCtx=0x0aea1480, const nsRect & aTextRect={...}, nsCSSShadowItem * aShadowDetails=0x0fca5874, const unsigned int & aForegroundColor=4294967295, const nsRect & aDirtyRect={...})  Line 616C++
 xul.dll!nsTextBoxFrame::PaintTitle(nsIRenderingContext & aRenderingContext={...}, const nsRect & aDirtyRect={...}, nsPoint aPt={...})  Line 402C++
 xul.dll!nsDisplayXULTextBox::Paint(nsDisplayListBuilder * aBuilder=0x0030c338, nsIRenderingContext * aCtx=0x0fca5c38)  Line 359C++
 xul.dll!mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer * aLayer=0x0b3ec170, gfxContext * aContext=0x0aea1480, const nsIntRegion & aRegionToDraw={...}, const nsIntRegion & aRegionToInvalidate={...}, void * aCallbackData=0x0030c338)  Line 1697C++
 xul.dll!mozilla::layers::BasicThebesLayer::Paint(gfxContext * aContext=0x0aea1480, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 484 + 0x28 bytesC++
 xul.dll!mozilla::layers::BasicLayerManager::PaintLayer(mozilla::layers::Layer * aLayer=0x0b3ec170, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 1287C++
 xul.dll!mozilla::layers::BasicLayerManager::PaintLayer(mozilla::layers::Layer * aLayer=0x0b3ebfb0, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 1290C++
 xul.dll!mozilla::layers::BasicLayerManager::EndTransaction(void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 1204C++
 xul.dll!mozilla::layers::BasicShadowLayerManager::EndTransaction(void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 2514C++
 xul.dll!nsDisplayList::PaintForFrame(nsDisplayListBuilder * aBuilder=0x0030c338, nsIRenderingContext * aCtx=0x00000000, nsIFrame * aForFrame=0x09485a20, unsigned int aFlags=1)  Line 478C++
 xul.dll!nsDisplayList::PaintRoot(nsDisplayListBuilder * aBuilder=0x0030c338, nsIRenderingContext * aCtx=0x00000000, unsigned int aFlags=1)  Line 385C++
 xul.dll!nsLayoutUtils::PaintFrame(nsIRenderingContext * aRenderingContext=0x00000000, nsIFrame * aFrame=0x09485a20, const nsRegion & aDirtyRegion={...}, unsigned int aBackstop=0, unsigned int aFlags=4)  Line 1436C++
 xul.dll!PresShell::Paint(nsIView * aDisplayRoot=0x09483538, nsIView * aViewToPaint=0x09483538, nsIWidget * aWidgetToPaint=0x093a3548, const nsRegion & aDirtyRegion={...}, const nsIntRegion & aIntDirtyRegion={...}, int aPaintDefaultBackground=0, int aWillSendDidPaint=0)  Line 6095 + 0x15 bytesC++
 xul.dll!nsViewManager::RenderViews(nsView * aView=0x09483538, nsIWidget * aWidget=0x093a3548, const nsRegion & aRegion={...}, const nsIntRegion & aIntRegion={...}, int aPaintDefaultBackground=0, int aWillSendDidPaint=0)  Line 448C++
 xul.dll!nsViewManager::Refresh(nsView * aView=0x09483538, nsIWidget * aWidget=0x093a3548, const nsIntRegion & aRegion={...}, unsigned int aUpdateFlags=1)  Line 415C++
 xul.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0030ca7c, nsIView * aView=0x09483538, nsEventStatus * aStatus=0x0030c874)  Line 914C++
 xul.dll!AttachedHandleEvent(nsGUIEvent * aEvent=0x0030ca7c)  Line 194C++
 xul.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0030ca7c, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 3617 + 0xc bytesC++
 xul.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0030ca7c, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 3649C++
 xul.dll!nsWindow::OnPaint(HDC__ * aDC=0x00000000, unsigned int aNestingLevel=0)  Line 483 + 0x24 bytesC++
 xul.dll!nsWindow::ProcessMessage(unsigned int msg=15, unsigned int & wParam=0, long & lParam=0, long * aRetValue=0x0030d29c)  Line 4868 + 0xf bytesC++
 xul.dll!nsWindow::WindowProcInternal(HWND__ * hWnd=0x000f0506, unsigned int msg=15, unsigned int wParam=0, long lParam=0)  Line 4457 + 0x20 bytesC++
 xul.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000f0506, unsigned int msg=15, unsigned int wParam=0, long lParam=0)  Line 4409 + 0x15 bytesC++

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, nsTextBoxFrame, of course.

The problem is that text for shadows is painted to an image surface so we can get the alpha channel and blur it. And nsTextBoxFrame goes through nsIRenderingContext::DrawString which creates the textrun and paints it in one operation. It needs to work like nsTextFrame instead: call MakeTextRun directly to make the textrun (passing GetReferenceRenderingContext), then render the textrun to the destination context using gfxTextRun::Draw.

Comment 25

[Krisztian Paczari]

Not only the Firefox button changes, but the tab header text as well! Just look at the 2nd tab of the attachment called screenshot: https://bugzilla.mozilla.org/attachment.cgi?id=492067

I have no repro, but I have seen this a few times.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

I can't reproduce this, but this should fix it.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 502679 [details] [diff] [review]
fix

Jonathan, I guess you can tell me if this fixes it.

Comment 28

[Krisztian Paczari]

I built the trunk and reproduced the bug with lots of Win+D and Win+Up/Down (the AutoIt script did not work). Then applied the patch and I could not reproduce it. So this seems to fix it, but I will spend more time looking at it.

Does the fix render the button the same as before pixel by pixel or should I expect differences?
Does it fix the tab header text as well?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It should fix them all. Thanks.

The text shadows may be rendered slightly differently. That should be OK.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 502679 [details] [diff] [review]
fix

(We should really clean up the unneeded methods and parameters here sometime.)

>-    mCtx->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y);
>+    nsCOMPtr<nsIFontMetrics> metrics;
>+    mCtx->GetFontMetrics(*getter_AddRefs(metrics));
>+    nsIThebesFontMetrics* fm = static_cast<nsIThebesFontMetrics*>(metrics.get());
>+    fm->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y,
>+                   mCtx, mTextRunConstructionContext);

This means you're skipping the safe-length code in the rendering context.
That seems bad.  Or is it ok somehow?

>-       aRenderingContext.DrawString(mCroppedTitle, aTextRect.x, baseline);
>+       nsIThebesFontMetrics* fm = static_cast<nsIThebesFontMetrics*>(fontMet.get());
>+       fm->DrawString(mCroppedTitle.get(), mCroppedTitle.Length(),
>+                      aTextRect.x, baseline, &aRenderingContext, refContext.get());

Same here.

A fix for these might lead to wanting the new DrawString method wanting to be consistent with the others and take nsThebesRenderingContext. 


Otherwise this looks fine.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> Comment on attachment 502679 [details] [diff] [review]
> fix
> 
> (We should really clean up the unneeded methods and parameters here sometime.)
> 
> >-    mCtx->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y);
> >+    nsCOMPtr<nsIFontMetrics> metrics;
> >+    mCtx->GetFontMetrics(*getter_AddRefs(metrics));
> >+    nsIThebesFontMetrics* fm = static_cast<nsIThebesFontMetrics*>(metrics.get());
> >+    fm->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y,
> >+                   mCtx, mTextRunConstructionContext);
> 
> This means you're skipping the safe-length code in the rendering context.
> That seems bad.  Or is it ok somehow?

I think that code is no longer needed; gfxTextRuns can be arbitrary length. We now have code in the MakeTextRun implementations that automatically splits up text for processing.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 502679 [details] [diff] [review]
fix

ok, r=dbaron.

But at some point we should (a) clean up the unneeded splitting code and (b) clean up the unneeded/unused parameters to and variants of the DrawString methods.  It might be worth filing bugs on this.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We really should just get rid of all the nsIRenderingContext string code. Instead we can use nsLayoutUtils methods that operate on textruns and gfxContext directly.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/6c727b25207a

Comment 37

[Krisztian Paczari]

I can no longer reproduce it in the daily builds. Nice job!

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre