968039 - UITour: The first highlight is positioned too far right and down

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot of initial highlight on the menu button

The first highlight after startup (e.g. on the menu button) is offset to the right and down from where it should be.

Comment 1

[Matthew N. [:MattN]]

Attachment: Investigation patch

Here is a patch on top of bug 943765 to debug this. The problem seems to be that containerStyle.paddingTop and containerStyle.paddingLeft are 0 the first time even though the CSS for the padding is in the browser's skin stylesheet from the beginning[1]. I tried triggering layout flushes but the values are always 0 the first time the panel is shown and then they are correct afterwards.

Neil, I don't know what else we could be doing to cause that (I checked visibility and display properties too) so I'm thinking this is a layout/XUL bug. Any ideas?

[1] https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/UITour.inc.css?rev=bf25e29dc677&mark=13-14#9

Comment 2

[Neil Deakin]

Attachment: synclayoutpopup

There are two issues here:

- first, the frames for a panel aren't generated until it is opened. You can set a type attribute on the panel, (the value isn't important) to force the frames to be generated right away, (or at least once you set hidden = false)

- second, is that the computed padding is 0 if the frame has the NS_FRAME_FIRST_REFLOW bit set. The popup should be clearing this bit if it isn't open during layout. This is done by calling SyncLayout which I think is ok here.

Comment 3

[Matthew N. [:MattN]]

Alex, if this doesn't make it into the Aurora build, you can workaround this with a second call to Mozilla.UITour.showHighlight("appMenu") either immediately after, or if that doesn't work, with a hideHighlight() in-between.

Comment 4

[Neil Deakin]

Comment on attachment 8370800 [details] [diff] [review]
synclayoutpopup

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

One line xul layout fix

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8370800 [details] [diff] [review]
synclayoutpopup

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

::: browser/base/content/browser.xul
@@ +218,5 @@
>          </vbox>
>        </hbox>
>      </panel>
>      <panel id="UITourHighlightContainer"
> +           type="pregen"

"pregen" is not an ideal name but I don't know a better one off-hand. Maybe pregenerate[d]? would be more clear. Maybe even just type="default"?

Please add a comment to indicate what that type is there for otherwise someone may think it's an error and remove it and regress this fix.

Is it not possible to avoid the attribute and detect that we need the frames for the getComputedStyle call? This is fine for now though.

Also note: we need to make sure that this doesn't add extra work before the hidden="true" is removed as there was a performance regression when that attribute was missing.

Comment 6

[Matthew N. [:MattN]]

Thanks again for the various panel fixes :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8370800 [details] [diff] [review]
synclayoutpopup

If all we want to do is clear the bit, why aren't we just clearing the bit?  If we really need a SyncLayout here, I'd like to see a good comment explaining why...

Comment 8

[Neil Deakin]

> If all we want to do is clear the bit, why aren't we just clearing the bit? 
> If we really need a SyncLayout here, I'd like to see a good comment
> explaining why...

That was my original plan, but I thought SyncLayout may have done other work that was needed. I will change this to just remove the bit. Do you think I should be clearing all four of the bits that SyncLayout clears or just NS_FRAME_FIRST_REFLOW?

Comment 9

[Alex Gibson [:agibson]]

(In reply to Matthew N. [:MattN] from comment #3)
> Alex, if this doesn't make it into the Aurora build, you can workaround this
> with a second call to Mozilla.UITour.showHighlight("appMenu") either
> immediately after, or if that doesn't work, with a hideHighlight()
> in-between.

Thanks, Matt

I've tested this temp fix and it seems to work. I'll make a pull request shortly to update this in the tour, so we should be covered either way.

Comment 10

[Neil Deakin]

Attachment: synclayoutpopup

Comment 11

[Boris Zbarsky [:bzbarsky]]

> but I thought SyncLayout may have done other work that was needed.

That's possible, yes...  Why are we trying to remove the flag, exactly?  Generally, actaully doing a reflow is in fact the right thing to get rid of the flag...

Comment 12

[Neil Deakin]

> That's possible, yes...  Why are we trying to remove the flag, exactly? 
> Generally, actaully doing a reflow is in fact the right thing to get rid of
> the flag...

This is during reflow. The is the return early case when a popup isn't open. The popup will reflow again when it gets opened.

Comment 13

[[github robot]]

Commits pushed to master at https://github.com/mozilla/bedrock

https://github.com/mozilla/bedrock/commit/fc45f7bd2e179898347eb833a4f0bb2cefee0b6f
[bug 968039] Workaround for first highlight position in tour

https://github.com/mozilla/bedrock/commit/bc25b04ad064f6ea113c2d0044cce6aecf204c68
Merge pull request #1667 from alexgibson/tour-first-highlight-fix-positioning

[bug 968039] Workaround for first highlight position in Australis tour

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8371440 [details] [diff] [review]
synclayoutpopup

This seems no worse than what we had before, I guess.  As long as we end up doing a full NS_FRAME_IS_DIRTY layout when the popup opens, r=me

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This was inadvertently landed directly on m-c, but we're just going to go with it.
https://hg.mozilla.org/mozilla-central/rev/97072008658c

Comment 16

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 30
Build Id: 20140304030204
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:30.0) Gecko/20100101 Firefox/30.0
OS: Mac Os X 10.8.5

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8371440 [details] [diff] [review]
synclayoutpopup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): UITour feature
User impact if declined: The first highlight will be positioned incorrectly
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk considering I haven't seen bugs filed and this has been on m-c for about 12 days.
String or IDL/UUID changes made by this patch: None

Neil, let me know if you have objections to uplift.

Comment 18

[Neil Deakin]

Seems ok.

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac058edf8305

Comment 20

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 29.0b4
Build id: 20140331125246
Mac Os 10.9