[User Story] View Site Name

RESOLVED FIXED in FxOS-S4 (07Aug)

Status

Firefox OS
Gaia::System::Browser Chrome
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: benfrancis, Assigned: benfrancis, NeedInfo)

Tracking

({feature})

unspecified
FxOS-S4 (07Aug)
ARM
Gonk (Firefox OS)
feature

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

User Story

As a user I want to view the name/domain name of the current site, so I know what site I'm on.

UX spec: https://docs.google.com/presentation/d/17CGWPwu59GB7miyY1ErTjr4Wb-kS-rM7dB3MAMVO9HU/pub?start=false&loop=false&delayms=3000#slide=id.p

More detail: https://docs.google.com/presentation/d/19BcRbnHqAifE_Cp21KkfxRswNR5s9dfq_HGBH43Bbhs/pub?start=false&loop=false&delayms=3000 (Appendix -> Pinned Sites -> Name and Icon)

Attachments

(2 attachments, 1 obsolete attachment)

46 bytes, text/x-github-pull-request
alive
: review+
etienne
: review+
Details | Review | Splinter Review
151.44 KB, application/pdf
Details
(Assignee)

Description

3 years ago
As a user I want to view the name/domain name of the current site, so I know what site I'm on.

Should use app name from manifest, application-name meta tag or hostname. Not document title.

Comment 1

3 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/95560448
(Assignee)

Updated

3 years ago
Assignee: nobody → bfrancis
(Assignee)

Comment 2

3 years ago
Created attachment 8620431 [details] [review]
WIP Patch
(Assignee)

Comment 3

3 years ago
Created attachment 8621156 [details] [review]
Patch

Hi Alive, would you be able to take a look at this?

This is the first part of the Pinning the Web [1] work to merge app windows and browser windows. All windows show either the app name or hostname in the Rocketbar title, the document title of individual pages will be used elsewhere (e.g. in the task manager).

This patch moves a lot of the state management out of AppChrome into AppWindow and simplifies the name/title handling to two separate properties.

I've fixed the unit tests but I still need to fix integration and UI tests before this can land (I can't currently get them to run locally).

Please let me know what you think!

1. https://wiki.mozilla.org/FirefoxOS/Pinning_the_Web
Attachment #8620431 - Attachment is obsolete: true
Attachment #8621156 - Flags: review?(alegnadise+moz)
Gimme some time to read or let's see if Etienne has time to review.
Comment on attachment 8621156 [details] [review]
Patch

Will read tomorrow but let's see if Etienne could pick up before I finished
Attachment #8621156 - Flags: review?(etienne)
Comment on attachment 8621156 [details] [review]
Patch

Basically looks fine, but I think this will change the behavior when the page location is changed. Is this defined somewhere?
Attachment #8621156 - Flags: review?(alegnadise+moz) → review+
Comment on attachment 8621156 [details] [review]
Patch

Made a few comments on github, would like to take one last look after that.
Do we have any UX support to go though the behavior changes? (At least the "Search the web" removal)
Attachment #8621156 - Flags: review?(etienne)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8621156 [details] [review]
Patch

Thanks Alive and Etienne, I'm currently blocked on getting integration tests running locally but will address the review comments as soon as I can.

Jacqueline, in the mean time can you confirm that it's OK to go ahead with the UI changes as per the Pin the Web presentation:
* Unfocused Rocketbar always shows app name (or hostname if no app name provided), for both app windows and browser windows
* The page title is never used to represent a site, but will be used to represent individual pages in the task manager (and in future edge swipe overlays and pinned pages)
Attachment #8621156 - Flags: ui-review?(jsavory)
(Assignee)

Comment 9

3 years ago
I've addressed the review comments, just waiting to rebase on bug 1174813 (which is going to conflict a little) before finishing off the tests. (I can run integration tests now!)
(Assignee)

Comment 10

3 years ago
Rebased after bug 1174813 landed and fixed unit tests including review comments. Now just integration tests to go...
(Assignee)

Comment 11

3 years ago
Comment on attachment 8621156 [details] [review]
Patch

Etienne, can you give this another check over?

I've can't actually run integration tests locally at the moment but I think I may have managed to fix the failing browser chrome tests on Treeherder...
Attachment #8621156 - Flags: review?(etienne)
(Assignee)

Updated

3 years ago
Flags: needinfo?(jsavory)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8621156 [details] [review]
Patch

Francis, are you taking over the UX on this?

I'm just looking for a quick UI review on a change to the way site names are displayed.

The designs we're currently working from display an application name in the title bar from a page's meta tags or manifest where provided, but will fall back to showing the hostname for web sites with no app metadata. This is instead of showing the document title.

The thinking is that this system wide title bar always shows the site/app name alongside the site/app icon (in both expanded and collapsed mode), the document title is used elsewhere to identify individual pages of a site/app, like in the task manager or on pinned cards.

This helps with consistently separating site metadata from page metadata across the OS. The hostname or app name will usually fit in the title bar, whereas document titles currently usually have to be truncated due to a lack of space. Showing the hostname instead of the document title in browser chrome is similar to what Safari does and I'm sure I've seen this in one of our UX specs at some point, I just wanted you to sanity check it before we land.

Thanks
Flags: needinfo?(jsavory) → needinfo?(fdjabri)
Attachment #8621156 - Flags: ui-review?(jsavory) → ui-review?(fdjabri)
Comment on attachment 8621156 [details] [review]
Patch

Sorry it took so long. But spending time on the initial pin-the-web patches will probably help me do subsequent reviews faster :)

Happy with this patch and the integration test failure looks trivial to fix (another title value to change), so this is officially in the hands for Francis :)
Attachment #8621156 - Flags: review?(etienne) → review+
(Assignee)

Comment 14

3 years ago
OK, this patch has been hanging around for a month now and waiting for UI review for over 3 weeks. It matches the UX spec we're currently working from and Treeherder is green so in the interests of moving forward I'm going to go ahead and land it before it bitrots again.

Let's get feedback and then iterate, thanks.
User Story: (updated)
(Assignee)

Updated

3 years ago
Attachment #8621156 - Flags: ui-review?(fdjabri)
(Assignee)

Comment 15

3 years ago
https://github.com/mozilla-b2g/gaia/commit/dda7775cea807343b69ef233a41bbddaba48620e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

3 years ago
Created attachment 8644324 [details]
View Site Name - Specification
Target Milestone: --- → FxOS-S4 (07Aug)
You need to log in before you can comment on or make changes to this bug.