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

Status: VERIFIED FIXED

(Firefox :: Tours, defect, P2)

Description

[Matthew N. [:MattN]]

+++ This bug was initially created as a clone of Bug #968039 +++

The first highlight after startup (e.g. on the menu button or the account Status) is offset to the right and down from where it should be. This has regressed since bug 968039 I think and it's affecting our current Sync marketing at https://www.mozilla.org/en-US/firefox/31.0/whatsnew/?oldversion=29.0.1

Comment 1

[Alex Gibson [:agibson]]

Thanks for the heads up - I'll add a second call to highlight sync in the menu panel as a workaround until this is fixed.

Comment 2

[Matthew N. [:MattN]]

Moving open UITour bugs to Firefox::Tours. Filter on firefox-tours-20150121.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Is this still an issue?

Comment 4

[Alex Gibson [:agibson]]

I can answer this - yes the bug is still present, would be great to get this one fixed sometime :)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Can you provide some clear steps to reproduce so we can try to track down what caused this regression?

Comment 6

[Alex Gibson [:agibson]]

STR:

1.) Launch Firefox.
2.) Load the URL: https://www.mozilla.org/
2.) Open the Web Console (option + command + k).
3.) Paste the following JS snippet to trigger a UITour highlight on appMenu.

var event = new CustomEvent('mozUITour', {
    bubbles: true,
    detail: {
        action: 'showHighlight',
        data: {
            target: 'appMenu',
            effect: 'none'
        }
    }
});

document.dispatchEvent(event);

Expected results:

The highlight should be centered over the icon.

Actual results:

The highlight is off center (excecuting the above code a second time corrects the position).

Comment 7

[Ryan VanderMeulen [:RyanVM]]

With those STR, I'm seeing the problem on the 2014-03-01 nightly, i.e. from the day after bug 968039 landed. So I'm not having much luck confirming that this was fixed and re-regressed :(

Comment 8

[Alex Gibson [:agibson]]

As far as I'm aware this bug has always been present, it may not be a regression.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Matt, you marked this as a regression when you filed it. I'm afraid that I can't reproduce this ever being fixed and therefore can't track down when it regressed :(

Comment 10

[Alex Gibson [:agibson]]

If this was a regression, I would expect it to be somewhere between Firefox 28-29, as this is when we implemented the highlight styling for the original Australis redesign tour.

Comment 12

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Just did a little trace on the code:

In the function it uses getComputedStyle to get the padding width of highlighter's container (which is 4px for now). But at the very start, the container popup is hidden by "display: none", which makes getComputedStyle returns no padding at all.

Given UX is planning to change the style of highlight to met Photon's styling guide, I would propose to solve it by:

- Remove the padding of the container
- Remove the box-shadow of highlighter, which is the main reason that we need to do padding in its container
- Remove the logic of compensating the padding

and optionally Remove the border-radius and border, per UX's current opinion. but we can do it on another UI bug once the spec is confirmed.

Comment 13

[KM Lee [:rexboy] (inactivate after Sep. 22)]

..Or we just compensate the displacement by CSS margin, and remove the logic of compensating in javascript. That way we can even decreases the complexity of code, and seems all features works as well.
Let me try to give a patch.

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1049130 - UITour: Fixing highlight placed in the wrong place at first time.

Review commit: https://reviewboard.mozilla.org/r/161914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161914/

Comment 15

[KM Lee [:rexboy] (inactivate after Sep. 22)]

The patch just uses the solution I mentioned on comment 13. I ran tests for UITours and they're still good.
Gijs would you take a look on the patch?
Thank you!

Comment 16

[:Gijs (he/him)]

Comment on attachment 8890716 [details]
Bug 1049130 - UITour: Fixing highlight placed in the wrong place at first time.

https://reviewboard.mozilla.org/r/161914/#review167242

::: browser/components/uitour/UITour.jsm:1146
(Diff revision 1)
> -      let offsetX = paddingTopPx
> -                      - (Math.max(0, highlightWidthWithMin - targetRect.width) / 2);
> +      let offsetX = - (Math.max(0, highlightWidthWithMin - targetRect.width) / 2);
> +      let offsetY = - (Math.max(0, highlightHeightWithMin - targetRect.height) / 2);

No space after the first '-'. eslint points this out. Please make sure to use it when you commit ( https://blog.mozilla.org/standard8/2017/05/04/running-eslint-on-commit-for-mozilla-central-git-mercurial/ )

Comment 17

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8890716 [details]
Bug 1049130 - UITour: Fixing highlight placed in the wrong place at first time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161914/diff/1-2/

Comment 18

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8890716 [details]
Bug 1049130 - UITour: Fixing highlight placed in the wrong place at first time.

https://reviewboard.mozilla.org/r/161914/#review167242

> No space after the first '-'. eslint points this out. Please make sure to use it when you commit ( https://blog.mozilla.org/standard8/2017/05/04/running-eslint-on-commit-for-mozilla-central-git-mercurial/ )

That's a handy tool, thanks for the information. The lint error above has been fixed.
I'll land it after re-running try server.
Thank you!

Comment 19

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8890716 [details]
Bug 1049130 - UITour: Fixing highlight placed in the wrong place at first time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161914/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24d786a6e756
UITour: Fixing highlight placed in the wrong place at first time. r=Gijs

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24d786a6e756

Comment 22

[Abe - QA (:Abe_LV)]

Hi KM Lee,
What is the expected behavior for a full-screen mode (F11)? 
The highlight shifts when the page enters into full screen.
Here is screen capture: https://testing-1.tinytake.com/sf/MTg1NDY4Ml81OTc3NDUw

Comment 23

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Hi Abe, thanks for finding out that issue.
I think that issue should be existed before this patch.
Please file another bug and you can put on [photon-onboarding][triage] in whiteboard so our team can discuss it in triage.
Thanks!

Comment 24

[KM Lee [:rexboy] (inactivate after Sep. 22)]

That should be a bug persisted for a long time inside UITour.

Comment 25

[Abe - QA (:Abe_LV)]

Created a bug for comment 22, Bug 1388824.

This issue is still reproducible using the STR provided in comment 6. 
What other steps do you suggest to verify this bug,1049130?

Comment 26

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Abe - QA (:Abe_LV) from comment #25)
> Created a bug for comment 22, Bug 1388824.
> 
> This issue is still reproducible using the STR provided in comment 6. 
> What other steps do you suggest to verify this bug,1049130?

Hi Abe:
That shouldn't happen. Could you confirm it again on a newer version?
If that's the case we may need to reopen/file a new bug.

The issue is not reproducible on Nightly for me anymore. It goes like this:
http://g.recordit.co/FXTzeBoZpe.gif

(I did that on about:home page, but the highlight action is done in browser itself, so they should be the same as in http://www.mozilla.org .)

Comment 27

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: screenshot after fixing

seems the image prohibits direct link. You can watch it here.

Comment 28

[Abe - QA (:Abe_LV)]

> That shouldn't happen. Could you confirm it again on a newer version?

It fails when the browser enters into full-screen mode.
Here is the screen capture: https://testing-1.tinytake.com/sf/MTg2MDgyNF81OTg5NzI1

> If that's the case we may need to reopen/file a new bug.
I am thinking to reopen this bug instead of creating a new one.

Comment 29

[:Gijs (he/him)]

(In reply to Abe - QA (:Abe_LV) from comment #28)
> > That shouldn't happen. Could you confirm it again on a newer version?
> 
> It fails when the browser enters into full-screen mode.
> Here is the screen capture:
> https://testing-1.tinytake.com/sf/MTg2MDgyNF81OTg5NzI1
> 
> > If that's the case we may need to reopen/file a new bug.
> I am thinking to reopen this bug instead of creating a new one.

The issue with the anchor not adjusting correctly for fullscreen due to the disappearing toolbars seems like something that would happen prior to this bug being fixed, too, and should therefore be a separate (new) bug.

Comment 30

[Abe - QA (:Abe_LV)]

A new bug is created Bug 1389177. The full-screen mode issue will be addressed in it.

This bug is verified as fixed then.

Comment 31

[Stefan [:StefanG_QA]]

Attachment: Highligh is shifted to the left on RTL-ar locale.JPG

Comment 32

[Stefan [:StefanG_QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 (20170823100553)

I was able to reproduce the bug on Ubuntu 16.04 x64 using latest Nightly build and RTL locale (ar).

This is not reproducible on Windows and OS X.

Please check the screenshot showing the issue.

Comment 33

[:Gijs (he/him)]

(In reply to Stefan [:StefanG_QA] from comment #32)
> Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
> (20170823100553)
> 
> I was able to reproduce the bug on Ubuntu 16.04 x64 using latest Nightly
> build and RTL locale (ar).
> 
> This is not reproducible on Windows and OS X.
> 
> Please check the screenshot showing the issue.

Is this specific to that locale and/or LTR? I don't think it's appropriate to reopen this bug for that issue - please file a new bug and make it block this one.

Comment 34

[Stefan [:StefanG_QA]]

This bug is verified as fixed per comment 25