Closed Bug 1261845 Opened 8 years ago Closed 8 years ago

Wrong background and tooltip for Hello icon during the tour

Categories

(Hello (Loop) :: Client, defect, P1)

defect

Tracking

(firefox45 unaffected, firefox46 affected, firefox47 affected, firefox48 affected)

RESOLVED FIXED
Iteration:
50.1 - Jun 20
Tracking Status
firefox45 --- unaffected
firefox46 --- affected
firefox47 --- affected
firefox48 --- affected

People

(Reporter: phorea, Assigned: crafuse)

References

Details

(Whiteboard: [btpp-fix-now][47])

Attachments

(5 files)

[Note]:
- Mac OS X 10.8.5 is not affected

[Affected versions]:
- Firefox 46 beta 7 with Hello 1.2.2
- latest Aurora 47.0a2 2016-04-04 with Hello 1.2.2
- latest Nightly 48.0a1 2016-04-03 with Hello 1.2.4

[Affected platforms]:
- Win 7 64-bit, Win XP 32-bit
- Ubuntu 12.04 32-bit

[Steps to reproduce]:
1. Select the Hello icon from toolbar - Hello panel opens
2. Click on Settings icon and select Tour option - Hello tour is opened
3. Hover the Hello icon

[Expected result]:
2 - While the tour is on screen, the Hello icon should be highlighted with a small rectangle (like Home Page button from the attached screenshot)
3 - Hello tooltip should be clear and explained

[Actual result]:
2 - Hello background exceeds the icon
3 - Tooltip reads as "loop-call-button3.tooltiptext"

[Additional notes]:
- I will search the regression range tomorrow
Rank: 19
Priority: -- → P1
Whiteboard: [btpp-fix-now]
The issue reproduces with Hello Version 1.1.2 [1] so this is not a regression.

[1] https://addons.mozilla.org/en-US/firefox/addon/firefox-hello-beta/versions/1.1.2
Assignee: nobody → crafuse
Added infobar tests to browser_LoopRooms_channel.js, fails on no room found after setting up a room and using ROOM_TOKEN to match the roomToken generated but fails.  It seems it is required to now have a room in the rooms array exist to test the infobar at all.  Original tests fail now.  Any suggestions on how to connect or fake a rooms list with the containing room?
Flags: needinfo?(standard8)
Flags: needinfo?(standard8)
Rank: 19 → 10
Whiteboard: [btpp-fix-now] → [btpp-fix-now][47]
Matching the hover icon background requested.  See next screen cap for active icon background(when panel is open) for reference.

Working on tooltip issues.
Attachment #8743557 - Flags: ui-review?(sfranks)
Attachment #8743557 - Flags: ui-review?(b.pmm)
Reference for when panel is open state and icon background.
Attachment #8743560 - Flags: review-
Comment on attachment 8743560 [details]
Hello Button Active Background

I don't think there should be any transparency on that tip there. You can see the border of the button behind it, which isn't right.

Or is it? I'm not so familiar with Linux styling. Are other panels the same?
Attachment #8743560 - Flags: ui-review-
Attachment #8743557 - Flags: ui-review?(sfranks)
Attachment #8743557 - Flags: ui-review?(b.pmm)
Attachment #8743557 - Flags: ui-review+
[Expected result]:
2 - While the tour is on screen, the Hello icon should be highlighted with a small rectangle (like Home Page button from the attached screenshot)
3 - Hello tooltip should be clear and explained

What should the menu button tooltip actually say when it is in slideshow state (Hello?)?
Here is a list of tooltip string properties that are available or we can add one:

# LOCALIZATION NOTE(loop-call-button3.label): This is a brand name, request
# approval before you change it.
loop-call-button3.label = Hello
loop-call-button3.tooltiptext2 = Browse this page with a friend
loop-call-button3-error.tooltiptext = Error!
loop-call-button3-donotdisturb.tooltiptext = Do not disturb
loop-call-button3-screensharing.tooltiptext = You are sharing your screen
loop-call-button3-active.tooltiptext2 = You are sharing your tabs
loop-call-button3-participantswaiting.tooltiptext2 = Someone is waiting for you
# LOCALIZATION NOTE(loop-call-button3-pb.tooltiptext): Shown when the button is
# placed inside a Private Browsing window. %S is the value of loop-call-button3.label.
loop-call-button3-pb.tooltiptext = %S is not available in Private Browsing

social-share-button.label = Share This Page
social-share-button.tooltiptext = Share this page
Flags: needinfo?(sfranks)
Flags: needinfo?(b.pmm)
Status: NEW → ASSIGNED
Just "Hello" is fine, thanks!
Flags: needinfo?(sfranks)
Flags: needinfo?(b.pmm)
In the tests I can not seem to get to a "slideshow" state, any suggestions on how to set this?
Flags: needinfo?(standard8)
https://github.com/mozilla/loop/pull/262/files might hold some hints. Though you probably need to open it from the menu within the panel.
Flags: needinfo?(standard8)
Comment on attachment 8743565 [details] [review]
[loop] chrafuse:bug-1261845-wrong-background-and-tooltip-for-Hello-icon-during-the-tour > mozilla:master

Tests have been added to dcritch/1250107-FTUPanelMochitests and has time-out issue.
Attachment #8743565 - Flags: review?(standard8)
Attachment #8743565 - Flags: review?(dmose)
Comment on attachment 8743565 [details] [review]
[loop] chrafuse:bug-1261845-wrong-background-and-tooltip-for-Hello-icon-during-the-tour > mozilla:master

This seems to regress the look of the icon in the tour mode on Mac.

Note: you can easily generate an xpi (make dist_xpi) that you can pass to others/attach for testing.
Attachment #8743565 - Flags: review?(standard8)
Attachment #8743565 - Flags: review?(dmose)
Attached image Bad icon on Mac
Here's the before (left) and after (right) on Mac.
Comment on attachment 8743565 [details] [review]
[loop] chrafuse:bug-1261845-wrong-background-and-tooltip-for-Hello-icon-during-the-tour > mozilla:master

Reverted change of removing background to button and not icon. Brute force approach apparently works.
Attachment #8743565 - Flags: review?(standard8)
Comment on attachment 8743565 [details] [review]
[loop] chrafuse:bug-1261845-wrong-background-and-tooltip-for-Hello-icon-during-the-tour > mozilla:master

Please see comments in PR, I think we need to make the new css specific to windows & linux only.
Attachment #8743565 - Flags: review?(standard8)
Flags: needinfo?(standard8)
Moved the css to the platform.css for windows and linux, reverted the loop.ccs file.  Tested locally. looks good.
Attachment #8743565 - Flags: review?(standard8)
Comment on attachment 8743565 [details] [review]
[loop] chrafuse:bug-1261845-wrong-background-and-tooltip-for-Hello-icon-during-the-tour > mozilla:master

r=Standard8 with the small nit addressed.
Flags: needinfo?(standard8)
Attachment #8743565 - Flags: review?(standard8) → review+
Rank: 10 → 22
Chris: It was a simple nit, so I addressed it for you and pushed, so we can get this out of the way.

Landed in the github repo:

https://github.com/mozilla/loop/commit/01aae65f7900d365dbd3d999b99811607b459475

We'll make this part of the next release.
Status: ASSIGNED → RESOLVED
Iteration: --- → 50.1
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: