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

VERIFIED FIXED in Firefox 56

Status

()

defect
P2
normal
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: MattN, Assigned: rexboy)

Tracking

(Depends on 1 bug, {regression})

29 Branch
Firefox 56
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [workaround with a 2nd showHighlight call][photon-onboarding])

Attachments

(3 attachments)

+++ 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
Flags: firefox-backlog+
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.
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Points: --- → 8
Moving open UITour bugs to Firefox::Tours. Filter on firefox-tours-20150121.
Component: General → Tours
Is this still an issue?
Flags: needinfo?(MattN+bmo)
I can answer this - yes the bug is still present, would be great to get this one fixed sometime :)
Flags: needinfo?(MattN+bmo)
Can you provide some clear steps to reproduce so we can try to track down what caused this regression?
Flags: needinfo?(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).
Flags: needinfo?(agibson)
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 :(
As far as I'm aware this bug has always been present, it may not be a regression.
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 :(
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
Duplicate of this bug: 1377171
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.
..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.
Assignee: nobody → rexboy
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!
See Also: → 1384841

Comment 16

2 years ago
mozreview-review
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/ )
Attachment #8890716 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Keywords: checkin-needed
Whiteboard: [workaround with a 2nd showHighlight call] → [workaround with a 2nd showHighlight call][photon-onboarding]

Comment 20

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24d786a6e756
UITour: Fixing highlight placed in the wrong place at first time. r=Gijs
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/24d786a6e756
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Version: unspecified → 29 Branch
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
Flags: needinfo?(rexboy)
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!
Flags: needinfo?(rexboy)
That should be a bug persisted for a long time inside UITour.
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?
Flags: needinfo?(rexboy)
See Also: → 1388824
(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 .)
Flags: needinfo?(rexboy)
seems the image prohibits direct link. You can watch it here.
> 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

2 years ago
(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.
A new bug is created Bug 1389177. The full-screen mode issue will be addressed in it.

This bug is verified as fixed then.
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
OS: All → Linux
Resolution: FIXED → ---

Comment 33

2 years ago
(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.
Flags: needinfo?(stefan.georgiev)
Depends on: 1393220
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Flags: needinfo?(stefan.georgiev)
OS: Linux → All
Resolution: --- → FIXED
This bug is verified as fixed per comment 25
Status: RESOLVED → VERIFIED
QA Contact: jwilliams
You need to log in before you can comment on or make changes to this bug.