Closed
Bug 968483
Opened 11 years ago
Closed 10 years ago
[Building Blocks] Update header design to accommodate larger back-button
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(feature-b2g:2.0, b2g-v2.0 fixed)
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | fixed |
People
(Reporter: caseyyee.ca, Assigned: mikehenrty)
References
Details
(Whiteboard: [systemsfe][p=8])
Attachments
(4 files, 14 obsolete files)
For 1.4 we will be modifying the header design to accommodate a larger back-button.
To support the larger back button and hit-state (increased to 5rem), we will be making a few additional changes:
- Center aligned header text.
- Increasing font weight to font-weight: 400
- Reduced header text sizing font-size: 2.2rem
Just want to get your OK on these screenshots so Pavel can proceed.
Attachment #8371057 -
Flags: ui-review?(pabratowski)
Comment 2•11 years ago
|
||
Comment on attachment 8371057 [details]
bug-968483.jpg
ok.
Attachment #8371057 -
Flags: ui-review?(pabratowski) → ui-review+
Comment 3•11 years ago
|
||
Hey Przemek,
what about subheaders? do we need to change the left space from 3rem to 5rem on them? I think that they look strange now :)
Attachment #8371272 -
Flags: feedback?(pabratowski)
Comment 4•11 years ago
|
||
Actually it's not big deal and in the most cases we don't use the subheaders directly after the header but I just want to raise a flag (I'm pretty sure that you guys already have this in mind :)
Comment 5•11 years ago
|
||
Attachment #8371883 -
Flags: review?(kyee)
Comment on attachment 8371883 [details]
patch for Gaia/master
Sorry, have to minus this one. Has lots of bugs still.
Attachment #8371883 -
Flags: review?(kyee) → review-
Attached are some issues that I experienced with the patch.
This one is tricky since the left navigation buttons are not always present. The markup layout may need to change to make this work.
Comment 8•11 years ago
|
||
Comment on attachment 8371883 [details]
patch for Gaia/master
I understand what is the problem ... I think it's fixed now :)
Attachment #8371883 -
Flags: review- → review+
Comment 9•11 years ago
|
||
(In reply to Pavel Ivanov [:ivanovpavel] from comment #3)
> Created attachment 8371272 [details]
> Shot
>
> Hey Przemek,
> what about subheaders? do we need to change the left space from 3rem to 5rem
> on them? I think that they look strange now :)
We decided not to center the subheaders for 1.4.
Comment 10•11 years ago
|
||
Comment on attachment 8371272 [details]
Shot
We decided not to center the subheaders for 1.4.
Comment 11•11 years ago
|
||
Comment on attachment 8371272 [details]
Shot
Make sure subheaders are left aligned with the rest of the settings text other than the headers.
Attachment #8371272 -
Flags: feedback?(pabratowski) → feedback-
Comment 12•11 years ago
|
||
Hey Przemek,
does that mean that this but is invalid?
Flags: needinfo?(pabratowski)
Reporter | ||
Comment 13•11 years ago
|
||
Pavel, the comment was regarding sub-header. We'll want to continue to proceed with centering the main header text.
Flags: needinfo?(pabratowski)
Reporter | ||
Comment 15•11 years ago
|
||
Pavel,
I'm still seeing all the issues that I'm seeing with my comments 7.
Flags: needinfo?(kyee)
Comment 16•11 years ago
|
||
Hey Casey,
are you sure that you flash your phone with my branch?
Flags: needinfo?(kyee)
Reporter | ||
Comment 17•11 years ago
|
||
Attachment #8372615 -
Attachment is obsolete: true
Flags: needinfo?(kyee)
Comment 18•11 years ago
|
||
Hey Casey,
I find that the [BB]Edit have some custom styles for [BB]Headers I think that this was fixed with the refresh of [BB]Headers ... I will open a bug for this and I will prepare a patch for it.
The other apps looks OK to me
Attachment #8376093 -
Flags: feedback?(kyee)
Reporter | ||
Comment 19•11 years ago
|
||
Comment on attachment 8376093 [details]
Shots
Yup, this would do it :)
Attachment #8376093 -
Flags: feedback?(kyee) → feedback+
Reporter | ||
Comment 20•11 years ago
|
||
Pavel, I'm still seeing alignment issues as per my screenshots in comment 17.
Comment 21•11 years ago
|
||
maybe you need the patch from Bug 972782 because we have [BB]Edit who has its own styles for headers
Comment 22•11 years ago
|
||
Have in mind that this patch not affect the [BB]Edit for now ... so all Apps in Edit Mode will not look like other Headers
Attachment #8380008 -
Flags: review?(kyee)
Reporter | ||
Comment 23•11 years ago
|
||
Comment on attachment 8380008 [details]
patch for Gaia/master - The CSS Version
The solution is still not complete. What we need is to have the header text center with the screen rather than the available space.
Pavel is continuing to look at CSS options but it seems likely that we will need some kind of JS to support this change.
Attachment #8380008 -
Flags: review?(kyee) → review-
Comment 24•11 years ago
|
||
the link of the new proposal is http://jsbin.com/bitebova/8/
Comment 25•11 years ago
|
||
I'm not sure what is needed here. None of the screenshots are consistent with the descriptions I'm reading.
In Pavel's link from comment 24, examples 3 and 6 are not centered.
My understanding is that the text should be centered to the screen. When it's bigger than the space, it should be centered with an ellipsis. And when it's in between, it should be left or right aligned (depending on which side has more buttons) without ellipses. Right?
Are we sure all buttons will have the same width? (That can help design a "hacky" solution)
Reporter | ||
Comment 26•11 years ago
|
||
As per email thread:
Hey everyone,
Pavel has been working hard on trying to get this to work in just pure CSS. This is the best that it can get without a Js solution:
http://jsbin.com/bitebova/20/
It's not exactly what we wanted, but I think it'll have to do. I personally think that its important to get a more useable back button and that it will be worth the miss-alignment for now. We can work on a follow-up bug and js based solution which gets us exactly what we want after we land the css solution. Pavel expects that the CSS solution as prototyped in the link above will take 1 week to land the patch + fixes that will be required in the apps to support the change.
Thoughts?
I've also included Eteinne and Anthony since I've asked them to provide suggestions on what we might be able to do in terms of a potential Js solution in Gaia. To get you up to speed on the discussion, here is the bug that is being discussed:
https://bugzilla.mozilla.org/show_bug.cgi?id=968483
Here is Pavels attempt at a Javascript solution:
http://pivanov.github.io/gaia/headers/index.html
The text on a few lines elllipses too early, but I do believe the fix shouldn't be too difficult of a problem to solve. I think where I would really like to get your help is how to best integrate this header styling logic into Gaia.
Reporter | ||
Comment 27•11 years ago
|
||
(In reply to Anthony Ricaud (:rik) from comment #25)
> I'm not sure what is needed here. None of the screenshots are consistent
> with the descriptions I'm reading.
> In Pavel's link from comment 24, examples 3 and 6 are not centered.
That's right. None of the solutions so far fit the requirement.
>
> My understanding is that the text should be centered to the screen. When
> it's bigger than the space, it should be centered with an ellipsis. And when
> it's in between, it should be left or right aligned (depending on which side
> has more buttons) without ellipses. Right?
The text should be centered to the screen width (not available space) as much as possible and ellipse when the string is too long.
>
> Are we sure all buttons will have the same width? (That can help design a
> "hacky" solution)
The left navigation buttons (back, close, menu) will be always 50px, but the right buttons can be of any width.
Comment 28•11 years ago
|
||
I'm coming into this late, and not entirely clear, but I can't see a reason for CSS from any of the mockups. Can you show me an example you're having trouble with a CSS implementation and I will see if I can help out?
Comment 29•11 years ago
|
||
The header should be center aligned, since most labels are shorter, it going to look like a bug to users... especially when each screen the header's center point jumps left or right.
The header label should be smart enough to know that if its over a certain number of characters and it may start to create an ellipsis than it will best align itself in the space provided (likely with a center point that is off center on the screen)
Flags: needinfo?(kgrandon)
Comment 30•11 years ago
|
||
Wouldn't one simple fix be to add a negative margin-left or negative left attribute to the centered text that matches the button width? I'm only just speculating here and have not really dug into the code yet (though I am more than happy to take a deeper look in the next few days).
Flags: needinfo?(kgrandon) → needinfo?(pivanov)
Comment 31•11 years ago
|
||
Hey Kevin,
I think that the negative margin is not an option ... but I will send you (maybe tomorrow) a patch to see what is the problem
Flags: needinfo?(pivanov)
Comment 32•11 years ago
|
||
Comment on attachment 8380008 [details]
patch for Gaia/master - The CSS Version
the known issues are:
1. when we have button/s with text who expand the "width" of the button
2. when we have 3 buttons and a looooong title (small space for the title but is not so bad I think)
3. worst case ... when you have point 1 and point 2 together (usually - [BB]Edit Mode)
Attachment #8380008 -
Flags: review?(kyee)
Attachment #8380008 -
Flags: review-
Attachment #8380008 -
Flags: feedback?(kgrandon)
Comment 33•11 years ago
|
||
Comment on attachment 8380008 [details]
patch for Gaia/master - The CSS Version
I didn't try it on a device or anything, but seems like it should be fine. I would recommend using a class name instead of data-role, those are indexed and should perform better for selector matching iirc. (Faster css is generally better). I would be fine with button.navigation, or button.action selectors. Though most of BB is not too performant, so it's not a deal-breaker for me.
Attachment #8380008 -
Flags: feedback?(kgrandon) → feedback+
Reporter | ||
Comment 34•11 years ago
|
||
Comment on attachment 8380008 [details]
patch for Gaia/master - The CSS Version
As per IRC conversation. Ellipsing before available space runs out.
Attachment #8380008 -
Flags: review?(kyee) → review-
Reporter | ||
Comment 35•11 years ago
|
||
Screen shot of issue as per comment 34
Comment 36•11 years ago
|
||
Can we extend that ellipses over... visually it looks like we easily have about 50 more pixels on the right?
Flags: needinfo?(kyee)
Comment 37•11 years ago
|
||
yep I think so ... we can try if you give me some numbers :)
Comment 38•11 years ago
|
||
Comment on attachment 8380008 [details]
patch for Gaia/master - The CSS Version
This PR contains all "apps/" except "contacts" there are some js logic in headers and I will need help for it
P.S. as we talk on irc the "[BB]Headers" now have centered title to available space
Attachment #8380008 -
Flags: review- → review?(kyee)
Comment 39•11 years ago
|
||
and one more thing we use "class name" instead of "data-role" now. I agreed with Kevin comment :)
Comment 40•11 years ago
|
||
Did we look at the impact this will have on localized versions of Firefox OS?
No longer depends on: 979483
Reporter | ||
Comment 41•11 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #40)
> Did we look at the impact this will have on localized versions of Firefox OS?
Hi Axel,
We have been careful in making sure that we maintain the same amount of string space as we've always had by reducing the header text sizing. RTL has also been considered in this change.
Flags: needinfo?(kyee)
Updated•11 years ago
|
Attachment #8371883 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8380008 -
Attachment is obsolete: true
Attachment #8380008 -
Flags: review?(kyee)
Comment 42•11 years ago
|
||
Hey guys sore for the big noise of this till now. I will add explanation of what we want for 1.4 of [BB]headers, [BB]Edit_mode after few minutes so we can keep the discussion here. Sorry again and Thanks in advanced for the reviews
Attachment #8388444 -
Flags: ui-review?(kyee)
Attachment #8388444 -
Flags: superreview?(kaze)
Attachment #8388444 -
Flags: review?(arnau)
Attachment #8388444 -
Flags: feedback?(kgrandon)
Updated•11 years ago
|
Attachment #8383988 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8376093 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8375647 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8371286 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8371272 -
Attachment is obsolete: true
Comment 43•11 years ago
|
||
Kaze is out on PTO all week and, due to the Bugzilla attachment issues, I cannot flag Vivien or someone else on the superreview? Will ping people in IRC to find someone else to take this.
Comment 44•11 years ago
|
||
Vivien, I am flagging you on needinfo? as I cannot change the review? flag due to Bugzilla attachment issues today. Kaze is out on PTO this week and this patch needs to land ASAP so it can be tested in advance of Friday. Can you help or is there someone else you can suggest? Thanks!
Flags: needinfo?(21)
Comment 45•11 years ago
|
||
Vivien, I am flagging you on needinfo? as I cannot change the superreview? flag due to Bugzilla attachment issues today. Kaze is out on PTO this week and this patch needs to land ASAP so it can be tested in advance of Friday. Can you help or is there someone else you can suggest? Thanks for any help you can provide here.
Comment 46•11 years ago
|
||
Comment on attachment 8388444 [details]
patch for Gaia/master
Ok, let some comments on the pull request. Overall I feel better about this patch than before - especially if we don't have to update app code. We do have to be *super* careful about this though, as I'm sure there are probably some regressions lurking about. (There always is when changing shared/ code). I wonder if we should loop in QA for some pre-emptive testing this week.
Attachment #8388444 -
Flags: feedback?(kgrandon) → feedback+
Updated•11 years ago
|
Attachment #8388444 -
Flags: superreview?(kaze) → superreview?(21)
Comment 47•11 years ago
|
||
Spec for [BB]Headers
Reporter | ||
Comment 48•11 years ago
|
||
Comment on attachment 8388444 [details]
patch for Gaia/master
Pavel, just make sure email is fixed.
r+
Attachment #8388444 -
Flags: ui-review?(kyee) → ui-review+
Comment 49•11 years ago
|
||
Hi Pavel - I am not familiar with the superreview? flag. Why do we need it here? Just curious. Thanks!
Comment 50•11 years ago
|
||
Comment on attachment 8388444 [details]
patch for Gaia/master
Based on this list, our superreview? flag should be set to David Baron.
https://www.mozilla.org/hacking/reviewers.html
Attachment #8388444 -
Flags: superreview?(21) → superreview?(dbaron)
Updated•11 years ago
|
Flags: needinfo?(21)
Comment 51•11 years ago
|
||
Comment on attachment 8388444 [details]
patch for Gaia/master
This patch does not require superreview; it's possible, however, that its author wanted review from a particular person for a particular reason, and used the superreview? flag rather than setting an additional review? flag.
I also don't see how https://www.mozilla.org/hacking/reviewers.html leads to me for a patch of this sort.
Attachment #8388444 -
Flags: superreview?(dbaron)
Comment 52•11 years ago
|
||
David, that is exactly what I was hoping you'd say. :)
Comment 53•11 years ago
|
||
Sorry for the superreview It was a mistake
Comment 54•11 years ago
|
||
It's totally fine Pavel - we are all learning. I wasn't sure until David cleared it up for me. :)
Attachment #8388444 -
Flags: review?(arnau) → review+
Comment 55•11 years ago
|
||
I had to revert this in https://github.com/mozilla-b2g/gaia/commit/884bdee3d631623f05d01646b3843ce8768b90f2 because it broke gaia-unit tests on tbpl: https://tbpl.mozilla.org/php/getParsedLog.php?id=35961023&tree=B2g-Inbound
Flags: needinfo?(pivanov)
Updated•11 years ago
|
Flags: needinfo?(kgrandon)
Updated•11 years ago
|
Attachment #8388444 -
Attachment is obsolete: true
Flags: needinfo?(pivanov)
Comment 57•11 years ago
|
||
Hey guys,
Armau and I found some problems with [BB]Edit_mode with pointer-events.
I fixed the patch and I made a new PR
Attachment #8389703 -
Flags: ui-review?(kyee)
Attachment #8389703 -
Flags: review?(arnau)
Attachment #8389703 -
Flags: feedback?(kgrandon)
Comment 58•11 years ago
|
||
I wonder if that could've caused the TBPL failures? Looking at them, it doesn't mention edit mode specifically, but if something was causing pointer-events to not work it could've possibly caused the failure.
Flags: needinfo?(kgrandon)
Kevin,
Is there a way we could do this same tests without merging the patch?
Comment 60•11 years ago
|
||
(In reply to Arnau March [:arnau] from comment #59)
> Kevin,
> Is there a way we could do this same tests without merging the patch?
You would have to use the gaia-try: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_Gaia
Comment on attachment 8389703 [details]
patch for Gaia/master
My concern regarding edit mode has been solved in this patch, let's see if that is what was crashing the gaia-unit tests.
Attachment #8389703 -
Flags: review?(arnau) → review+
Pavel, can you try Kevin's link?
Updated•11 years ago
|
Attachment #8389703 -
Flags: feedback?(kgrandon)
Comment 63•11 years ago
|
||
Hi guys! We have been trying to reproduce the issue using our forked repositories (in fact Arnau's one) and running Travis on them :-)
As you can see at https://travis-ci.org/rnowm/gaia/jobs/20612483 and more concretely at https://travis-ci.org/rnowm/gaia/jobs/20612483#L1827 the issue is related to expecting the element with id 'contact-name-title' (see https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/contacts/regions/contact_details.py#L11 and https://github.com/mozilla-b2g/gaia/blob/3b84e007c352f0afbe9d79ae21badbbf05984654/apps/communications/contacts/elements/details.html#L8) to have an x-location of 0. Since, as I have read throughout the bug is now centered, this condition may be no longer valid. So it is an issue with the test and not with the patch :-) Anyhow, since I am not familiarised with these tests (BTW, the error is always the same) I will need-info Dave Hunt who I saw has been providing part of the code for these tests.
I will prepare a JS integration test to check if this could be the issue and to have more information about it but I would say Dave will be able to provide us with much valuable information once he reads this comment :-)
Thanks!
Flags: needinfo?(dave.hunt)
Comment 64•11 years ago
|
||
I just confirmed that the x-location of the element with id 'contact-name-title' is now (after applying Pavel's patch) 50 and not 0 and consequently that condition is never met and, as a matter of fact, the element searching timeout fires :-)
No need to say, that I am not suggesting to substitute the 0 in the test by 50 (since this may change in the future and we would have the same problem) but a condition as general as possible (probably not dependant on elements locations :p) to validate that the conditions needed by the test to run and to pass are the correct ones ;-)
I'm sure Dave will know what I am talking about and also what to do :-)
Comment 65•11 years ago
|
||
In fact, the 50 I am getting is for the case when the form is shown to add a new contact, the language is English and the text shown at the header title is "Add contact" :-)
Thanks German. I guess 50 is the distance from the left to where the title starts. As in this patch we are centering all titles, that distance will change with the length of the word.
Is it possible to disable this test as it seems is no longer valid for our UI?
Comment 67•11 years ago
|
||
And can we land this patch in the mean time?
Comment 68•11 years ago
|
||
Who is the person who needs to disable the test? Is Dave Hunt the right person or do we need to ni? someone else.
Casey, how is the ui-review coming? This needs to move.
Comment 69•11 years ago
|
||
Comment 70•11 years ago
|
||
Flagging Kevin to see if he can comment on test changes, and Jason.
Comment 71•11 years ago
|
||
Flagging Fabrice also since I asked Pavel to land in the meantime, while test issues are tracked down, since we are racing the clock here.
Flags: needinfo?(fabrice)
Attachment #8389703 -
Flags: ui-review?(kyee) → ui-review+
Comment 72•11 years ago
|
||
Wait, what? We re-landed this *BEFORE* addressing the test issues? That's not acceptable.
Comment 73•11 years ago
|
||
if we landed with expected bustage, please backout
Flags: needinfo?(fabrice)
Comment 74•11 years ago
|
||
Commits need to be atomic in nature, so we do need to land with a working test. This probably needs to be backed out and re-landed with a test when it fails. I would recommend Zac or someone from the A-team to update the test. I am fairly slammed with things, but if this is critical I could also try to find time this week.
Comment 75•11 years ago
|
||
And it did indeed still fail: https://tbpl.mozilla.org/php/getParsedLog.php?id=36018564&tree=B2g-Inbound
Comment 77•11 years ago
|
||
Flagging Zac as he is on A-team. Also trying to find out who exactly is on A-team to see if someone in an earlier time zone can take this. If not, Zac, please let us know if you can help address our test change issues. We are under a lot of pressure to land this before Friday in good working order.
Flags: needinfo?(zcampbell)
Comment 78•11 years ago
|
||
Here is the report from the failure on TBPL:
31aca2e57d9eee6b712c5de68463b1cf6c6f3bf302d44fbeb33d6e559de1667ae3acd726ccd9a2b97ec1e1559c6d8b3b8c96b528b0f02f26222285e297b7a4b9
Gtorodelvalle's debug sounds correct.
I can't debug this myself until tomorrow, but if you want to try again now you might have success with changing the element locator of the wait to: find_element(*self._back_button_locator) as it appears to be at ['x']==0 relative to the screen.
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/contacts/regions/contact_details.py#L22
I'll keep my ni? in case this needs to be investigated tomorrow but if you fix it please clear it (and Dave's).
gtorodelvalle we have to wait very specifically like this often when there are slow transitions because there can be a race between Marionette calculating the location of the eleemnt (in order to perform and action like tap) and the final location of the element, it is very very sensitive!
Comment 79•11 years ago
|
||
I don't know if I get Zac's point, sorry :-) but yeah, yeah, we do these kind of checks in the Gaia integration tests (see https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/marionette/lib/contacts.js#L103 and also http://slid.es/gtorodelvalle/gaia-integration-tests#/30/0). The point is making these checks as independent as possible of the "environment" and the positioning of the elements, which BTW it is not always possible ;-)
I guess Zac is taking care of fixing the test. Is this right? :) I do not have the Marionette-Python environment set in my laptop to validate locally that they pass, I have to do it via Github + Travis :-(
Comment 80•11 years ago
|
||
I'm not fixing the test. It's your patch, you have to fix it. I just suggested a change that might work.
Gtorodelvalle, wait until you have to run these tests on a (slower) device too, I am sure lots of your waits will come crashing down!
Flags: needinfo?(zcampbell)
Comment 82•11 years ago
|
||
Hi guys I just debugged this locally and I confirmed that this line will work.
Put this on contact_details.py:22
self.wait_for_condition(lambda m: m.find_element(*self._back_button_locator).location['x'] == 0)
Cheers
Comment 83•11 years ago
|
||
Kevin, can you help to apply this change? If not Pavel can do it.
Flags: needinfo?(kgrandon)
Comment 84•11 years ago
|
||
Attachment #8390613 -
Flags: review?(kgrandon)
Comment 85•11 years ago
|
||
Comment on attachment 8390613 [details]
patch for Gaia/master - with fixed test
Zac provided the fix and we have a green travis so I feel pretty good about this.
Attachment #8390613 -
Flags: review?(kgrandon) → review+
Flags: needinfo?(kgrandon)
Comment 86•11 years ago
|
||
Thanks for the help guys :) Hope that this time everything will be OK
Landed to master:
https://github.com/mozilla-b2g/gaia/commit/a71c4bf0a0e8ef949b6c03cac1e3be1d957f126e
Comment 87•11 years ago
|
||
Sorry, this regresses the header position in the Messages app :(
Just launch the app and you'll see it.
Comment 88•11 years ago
|
||
(or maybe that's expected ? :) )
Comment 89•11 years ago
|
||
can you shot it?
Julien, please check the attached VD specs in this patch: https://bug968483.bugzilla.mozilla.org/attachment.cgi?id=8388782
What you see in messages app is what you are supposed to see for 1.4 :)
Comment 91•11 years ago
|
||
(In reply to Zac C (:zac) from comment #80)
> I'm not fixing the test. It's your patch, you have to fix it. I just
> suggested a change that might work.
>
> Gtorodelvalle, wait until you have to run these tests on a (slower) device
> too, I am sure lots of your waits will come crashing down!
No need to tell Zac that this was not my patch. I was the one who found the cause of the test not passing... ;-) You're welcome!
Comment 92•11 years ago
|
||
First - this should have been closed when it landed. Second - this caused a smoketest regression & a very ugly VD building blocks fallout. Naoki & I are in strong disagreement with comment 90 being a good idea, as the misalignment in the UX looks quite ugly on the phone.
Flagging checkin-needed for a backout. This also gets to wait until 1.5.
Keywords: checkin-needed
Reverted in https://github.com/mozilla-b2g/gaia/commit/d058b4851157dededea27157983087c1920d08da as per jsmith's comment 92.
Keywords: checkin-needed
Comment 94•11 years ago
|
||
Hey guys, we'd like to take a look at the smoke test results for this. Is there any way to point us to them?
Comment 95•11 years ago
|
||
Likewise for the "very ugly VD building blocks fallout," please. Casey has been testing on device and we just can't suss out what that comment means. If anyone has a screenshot, it would be helpful to us. Thanks!
Reporter | ||
Comment 96•11 years ago
|
||
Hi Jason,
As per Steph, I would like a clarification on what caused the smoke test failure.
Regarding alignment in comment 90 -- Which alignment option are you and Naoki in disagreement about? I would be more than happy to clarify the design direction with you guys (can do this offline from bug).
Comment 97•11 years ago
|
||
The relevant bugs that motivated the backout are:
* [smoketest blocker] bug 983810
* [smoketest blocker] bug 983837
* [ugly VD fallout] bug 983812
Comment 98•11 years ago
|
||
(In reply to Casey Yee [:cyee] from comment #96)
> Hi Jason,
> As per Steph, I would like a clarification on what caused the smoke test
> failure.
>
> Regarding alignment in comment 90 -- Which alignment option are you and
> Naoki in disagreement about? I would be more than happy to clarify the
> design direction with you guys (can do this offline from bug).
We can discuss this more offline if you like.
Updated•11 years ago
|
Comment 99•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #92)
> First - this should have been closed when it landed. Second - this caused a
> smoketest regression & a very ugly VD building blocks fallout. Naoki & I are
> in strong disagreement with comment 90 being a good idea, as the
> misalignment in the UX looks quite ugly on the phone.
>
> Flagging checkin-needed for a backout. This also gets to wait until 1.5.
Hi Jason- I appreciate your attention to detail. The design team for FxOS has reviewed this design very intensely.
As I commented, in https://bugzilla.mozilla.org/show_bug.cgi?id=983812 the usability of the back button is one of the top issues for the device. This is phase 1 of the header design which will be iterated on in the next release.
The "very ugly VD" comment is duly noted but we have decided to proceed with this change in 1.4.
Pavel, Casey, please restart the process to land.
Flags: needinfo?(pivanov)
Flags: needinfo?(kyee)
Comment 100•11 years ago
|
||
(In reply to jachen from comment #99)
> (In reply to Jason Smith [:jsmith] from comment #92)
> > First - this should have been closed when it landed. Second - this caused a
> > smoketest regression & a very ugly VD building blocks fallout. Naoki & I are
> > in strong disagreement with comment 90 being a good idea, as the
> > misalignment in the UX looks quite ugly on the phone.
> >
> > Flagging checkin-needed for a backout. This also gets to wait until 1.5.
>
> Hi Jason- I appreciate your attention to detail. The design team for FxOS
> has reviewed this design very intensely.
>
> As I commented, in https://bugzilla.mozilla.org/show_bug.cgi?id=983812 the
> usability of the back button is one of the top issues for the device. This
> is phase 1 of the header design which will be iterated on in the next
> release.
>
> The "very ugly VD" comment is duly noted but we have decided to proceed with
> this change in 1.4.
>
> Pavel, Casey, please restart the process to land.
I don't think so. Did you read the above comments about the two smoketest blockers that came out of this patch outside of that other VD issue? You aren't allowed to land for 1.4. We do not cause smoketest regressions on landings. Period.
Anyways, this is non-blocking work anyways that was only allowed to land if the regression rate was under control. The smoketest regressions clearly prove that's not true, so this gets to wait until 1.5.
Flags: needinfo?(pivanov)
Flags: needinfo?(kyee)
Updated•11 years ago
|
Comment 101•11 years ago
|
||
A *much* less risky patch would be to simply increase the sizing of the back button a bit. I would highly recommend opening a bug for only that, then we can worry about center alignment in another patch. It seems like this patch is far too ambitious, when we should do it in smaller chunks.
I do not think that this patch would be very risky - as the back button used to be larger in the past anyway. To be clear, I think there should be two bugs filed:
1 - Increase sizing of back button (this is low risk, and I don't see why it couldn't be in 1.4).
2 - Centering of headers (this is higher risk, and should not be in 1.4)
Comment 102•11 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #101)
> I do not think that this patch would be very risky - as the back button used
> to be larger in the past anyway. To be clear, I think there should be two
> bugs filed:
I disagree: impact on en-US could be minor, for localized builds they would be unknown until testing, most likely bad considering the amount of truncation problems we have. See also discussion in bug 979473.
Comment 103•11 years ago
|
||
We've already had the larger back button in the past, so the truncation issues should already be known. There may be some risk, but it's going to be a lot less than doing everything at once.
Comment 104•11 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #103)
> We've already had the larger back button in the past
Can you add some details? Which version are you referring to?
> There may be some risk, but it's going to be a lot less than doing everything at once.
In bug 979473 we were assured that in the end the available space wasn't going to change. In this, at least from what I understand, we're just reducing it, unless the plan is different (e.g. larger back button but reduce paddings or font size). This sounds like taking unknown risks to me.
Comment 105•11 years ago
|
||
Resolving this as WONTFIX for 1.4. The UX team did not land in time, and did not follow the process and timeline for testing that we agreed with QA.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Updated•11 years ago
|
blocking-b2g: --- → 1.3?
Updated•11 years ago
|
blocking-b2g: 1.3? → 1.3T?
Comment 106•11 years ago
|
||
I'm probably missing something, but how can a bug marked as wontfix for 1.4 be 1.3T?
Comment 107•11 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #106)
> I'm probably missing something, but how can a bug marked as wontfix for 1.4
> be 1.3T?
Yup that's right - this should have not been nomed. Way too risky for 1.3T anyways.
blocking-b2g: 1.3T? → -
Comment 108•11 years ago
|
||
Reopening since this was moved to 2.0. Flagging Candice to adjust my suggested 2.0 sprint placeholder to actually suit the Sys FE backlog, assigning to Sam as discussed during planning, and putting the blocking placeholder in as this blocks visual refresh and BB changes.
Assignee: pivanov → sjochimek
Status: RESOLVED → REOPENED
blocking-b2g: - → 2.0?
Flags: needinfo?(cserran)
Resolution: WONTFIX → ---
Target Milestone: --- → 2.0 S1 (9may)
Comment 109•11 years ago
|
||
Hey Stephany,
Do we have the new identified tasks/stories, that should be opened for these so we can make sure they get into our sprint planning? Also needinfo'ing Peter in case he has more info
Flags: needinfo?(swilkes)
Flags: needinfo?(pdolanjski)
Flags: needinfo?(cserran)
Updated•11 years ago
|
Whiteboard: [systemfe]
Updated•11 years ago
|
Whiteboard: [systemfe] → [systemsfe]
Assignee | ||
Comment 110•11 years ago
|
||
Just to reiterate what we need to do here:
Fix the following two problems *BEFORE* relanding (otherwise we will block QA, and so this would be immediately backed out again):
* [smoketest blocker] bug 983810
* [smoketest blocker] bug 983837
Based on comment 99, it seems UX is ok with an iterative approach to fixing the center alignment issues. So as long as we don't block smoketests or bust auto-tests, I think we can land this bug and then (quickly thereafter) fix the alignment issue from bug 983812. I will reopen that bug for those purposes.
* [ugly VD fallout] bug 983812
Assignee | ||
Updated•11 years ago
|
Comment 111•11 years ago
|
||
(In reply to Michael Henretty [:mhenretty] from comment #110)
>
> Based on comment 99, it seems UX is ok with an iterative approach to fixing
> the center alignment issues. So as long as we don't block smoketests or bust
> auto-tests, I think we can land this bug and then (quickly thereafter) fix
> the alignment issue from bug 983812. I will reopen that bug for those
> purposes.
> * [ugly VD fallout] bug 983812
I don't think that makes sense to me. We shouldn't forcibly cause a user-facing regression - it's going to cause noise to daily testers. If we get to the point where we have to make a call on whether we can live with the issue or not, then we can decide if this should land without bug 983812.
Comment 112•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #111)
> (In reply to Michael Henretty [:mhenretty] from comment #110)
> >
> > Based on comment 99, it seems UX is ok with an iterative approach to fixing
> > the center alignment issues. So as long as we don't block smoketests or bust
> > auto-tests, I think we can land this bug and then (quickly thereafter) fix
> > the alignment issue from bug 983812. I will reopen that bug for those
> > purposes.
> > * [ugly VD fallout] bug 983812
>
> I don't think that makes sense to me. We shouldn't forcibly cause a
> user-facing regression - it's going to cause noise to daily testers. If we
> get to the point where we have to make a call on whether we can live with
> the issue or not, then we can decide if this should land without bug 983812.
FWIW - I'd prefer that we minimize the halfway done VD refresh situations as much as possible. Our contracting team is already getting confused by some of the landings that halfway done, which is creating noise in Bugzilla.
Comment 113•11 years ago
|
||
Mike & I talked in IRC about this - it will be safer to land this & fix the other issue as a followup, so I'll reopen bug 983812. He'll email qa-b2g@mozilla.org when this lands to make sure that QA is aware of the known fallout.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [systemsfe] → [systemsfe][p=8]
Updated•11 years ago
|
Assignee: sjochimek → mhenretty
Comment 114•10 years ago
|
||
Clearing the nom here since we're going to track features with the feature-b2g flag.
blocking-b2g: 2.0? → ---
Assignee | ||
Comment 115•10 years ago
|
||
Uploading the latest spec and cleaning out outdated attachments.
Sadly attachment 8390613 [details] is very out of date with master, and will have to be almost entirely redone :(
Attachment #8371057 -
Attachment is obsolete: true
Attachment #8381473 -
Attachment is obsolete: true
Attachment #8388444 -
Attachment is obsolete: true
Attachment #8388782 -
Attachment is obsolete: true
Attachment #8389703 -
Attachment is obsolete: true
Assignee | ||
Comment 116•10 years ago
|
||
(In reply to Candice Serran (:cserran) from comment #109)
> Hey Stephany,
>
> Do we have the new identified tasks/stories, that should be opened for these
> so we can make sure they get into our sprint planning? Also needinfo'ing
> Peter in case he has more info
For this bug specifically we have dependent bug 983812, which we will need to fix soon after landing this one.
We also have the truncated headers bug 908300, which is related to the header refresh. Fortunately, this can be worked on separately. I've attached the latest spec, attachment 8419084 [details], so I think the bugs we have now are enough to complete that work. We can file more if we need them :)
Flags: needinfo?(swilkes)
Flags: needinfo?(pdolanjski)
Assignee | ||
Comment 117•10 years ago
|
||
Pavel, would you be able to help me rebase your PR attachment 8390613 [details] against the latest master? You probably know this code and the changes since better than anyone, so it would be great to have your help :)
At that point I can help with making sure all the auto/smoke tests pass, and with the landing process.
Flags: needinfo?(pivanov)
Assignee | ||
Comment 118•10 years ago
|
||
Just for informational purposes, :kgrandon is trying to drive changes in the web standards for css to allow us to not need javascript for things like centering text based on viewport, and auto-resizing font. He filed bug 1005348 to track that.
Comment 119•10 years ago
|
||
Thanks for linking that. It's something that would be super nice to have I think, but there's no guarantees on how long this would take or if it's even going to happen quite yet. My guess is that even if we were able to standardize something, it'd still be a year+ away. Whatever solution we do now needs to be performant and reusable.
Comment 120•10 years ago
|
||
Hey Michael,
here is the patch with the large back button ... and some small CSS improvements ping me if I can help with something else :)
Attachment #8419290 -
Flags: review?(mhenretty)
Flags: needinfo?(pivanov)
Assignee | ||
Comment 121•10 years ago
|
||
Comment on attachment 8419290 [details]
patch for Gaia/master
Thanks for the quick turnaround Pavel!
Looks like we have some tests to fix, I will be looking at that today.
Attachment #8419290 -
Attachment mime type: text/plain → text/x-github-pull-request
Attachment #8419290 -
Flags: review?(mhenretty) → feedback+
Updated•10 years ago
|
feature-b2g: --- → 2.0
Assignee | ||
Comment 122•10 years ago
|
||
This is a work in progress from Arnau that has been rebased against the recent header web component work (with a fix from me).
Attachment #8419290 -
Attachment is obsolete: true
Assignee | ||
Comment 123•10 years ago
|
||
Przemek, would you have a look at these screens? Keep in mind we are doing the proper centering in bug 983812, so this is just centering the text based on the container.
Attachment #8422709 -
Flags: ui-review?(pabratowski)
Assignee | ||
Comment 124•10 years ago
|
||
Comment on attachment 8421367 [details]
Github PR, rebased back button patch
Casey or Pavel, mind giving this patch a look?
Attachment #8421367 -
Flags: review?(pivanov)
Attachment #8421367 -
Flags: review?(kyee)
Comment 125•10 years ago
|
||
Comment on attachment 8421367 [details]
Github PR, rebased back button patch
LGTM :) Thanks :)
Attachment #8421367 -
Flags: review?(pivanov) → review+
Comment 126•10 years ago
|
||
Comment on attachment 8422709 [details]
spritesheet.png
What I'm seeing here looks good.
Attachment #8422709 -
Flags: ui-review?(pabratowski) → ui-review+
Updated•10 years ago
|
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Assignee | ||
Comment 127•10 years ago
|
||
I had QA run through the smoke tests with me, we found a slight style bug in the camera app. That is now fixed. We have ui-review+, code review+, and travis looks green. Time to land.
Assignee | ||
Comment 128•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Attachment #8421367 -
Flags: review?(kyee)
Comment 129•10 years ago
|
||
OK Jason - but just a note that this DOES block visual refresh for 2.0 AND the completion of the header. :) Header isn't done without this, in other words. Just tossing my $.02 into the pool.
(In reply to Michael Henretty [:mhenretty] from comment #127)
> I had QA run through the smoke tests with me, we found a slight style bug in
> the camera app.
I confirm. We found this only bug by running the smoketests.
Assignee | ||
Comment 131•10 years ago
|
||
(In reply to Stephany Wilkes from comment #129)
> OK Jason - but just a note that this DOES block visual refresh for 2.0 AND
> the completion of the header. :) Header isn't done without this, in other
> words. Just tossing my $.02 into the pool.
I'm not sure which comment you are replying to, but we also have bug 983812 to track the alignment issue, and that is indeed blocking 2.0.
Updated•10 years ago
|
status-b2g-v2.0:
--- → fixed
Updated•10 years ago
|
Flags: in-moztrap?(nhirata.bugzilla)
we have tons of cases using the back button; it's pretty noticeable when doing these tests if they are not aligned properly.
Flags: in-moztrap?(nhirata.bugzilla) → in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•