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

VERIFIED FIXED in Firefox 29

Status

()

defect
P2
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: MattN, Assigned: enndeakin)

Tracking

unspecified
Firefox 30
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P2][workaround with a 2nd showHighlight call])

Attachments

(2 attachments, 2 obsolete attachments)

The first highlight after startup (e.g. on the menu button) is offset to the right and down from where it should be.
Summary: UITour: The first highlight is position too far right and down → UITour: The first highlight is positioned too far right and down
Posted patch Investigation patch (obsolete) — Splinter Review
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
Attachment #8370586 - Flags: feedback?(enndeakin)
Assignee

Comment 2

6 years ago
Posted patch synclayoutpopup (obsolete) — Splinter Review
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.
Assignee: MattN+bmo → enndeakin
Attachment #8370586 - Attachment is obsolete: true
Attachment #8370586 - Flags: feedback?(enndeakin)
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.
Flags: needinfo?(agibson)
Whiteboard: [workaround with a 2nd showHighlight call]
Assignee

Comment 4

6 years ago
Comment on attachment 8370800 [details] [diff] [review]
synclayoutpopup

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

One line xul layout fix
Attachment #8370800 - Flags: review?(bzbarsky)
Assignee

Updated

6 years ago
Attachment #8370800 - Flags: review?(MattN+bmo)
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.
Attachment #8370800 - Flags: review?(MattN+bmo) → review+
Thanks again for the various panel fixes :)
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...
Assignee

Comment 8

6 years ago
> 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?
(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.
Flags: needinfo?(agibson)
Assignee

Comment 10

6 years ago
Attachment #8370800 - Attachment is obsolete: true
Attachment #8370800 - Flags: review?(bzbarsky)
Attachment #8371440 - Flags: review?(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...
Assignee

Comment 12

6 years ago
> 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

6 years ago
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 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
Attachment #8371440 - Flags: review?(bzbarsky) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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 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.
Attachment #8371440 - Flags: approval-mozilla-aurora?
Flags: needinfo?(enndeakin)
Assignee

Comment 18

5 years ago
Seems ok.
Flags: needinfo?(enndeakin)
Attachment #8371440 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [workaround with a 2nd showHighlight call] → [Australis:P?][workaround with a 2nd showHighlight call]
Whiteboard: [Australis:P?][workaround with a 2nd showHighlight call] → [Australis:P2][workaround with a 2nd showHighlight call]
Verified as fixed using the following environment:

FF 29.0b4
Build id: 20140331125246
Mac Os 10.9
Status: RESOLVED → VERIFIED
Depends on: 1049130
You need to log in before you can comment on or make changes to this bug.