Closed Bug 1174702 Opened 5 years ago Closed 5 years ago

Unnecessary scroll while editing context.

Categories

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

defect
Points:
3

Tracking

(firefox40- wontfix, firefox41+ wontfix, firefox42+ fixed, firefox43 fixed)

RESOLVED FIXED
mozilla43
Iteration:
43.2 - Sep 7
Tracking Status
firefox40 - wontfix
firefox41 + wontfix
firefox42 + fixed
firefox43 --- fixed

People

(Reporter: bogdan_maris, Assigned: standard8)

References

Details

(Whiteboard: [context])

Attachments

(4 files)

Attached image Gif showing the issue
Affected builds:
- Latest Nightly 41.0a1
- Latest Aurora 40.0a2

Affected OS`s:
- Windows 7 64-bit
- Ubuntu 14.04 32-bit

Unaffected OS`s:
- Mac OS X 10.9.5

STR:
1. Start Firefox
2. Click Hello icon
3. Check 'Let`s talk about' and start a Conversation
4. Enter 'Edit context'

Expected results: No extra scroll appears in conversation window.

Actual results: A scrollbar shows in conversation window even though it is not needed.

Notes:
- Gif with the issue attached.
- This is not a regression, it reproduces after context layout changes in bug 1162909.
[Tracking Requested - why for this release]:
Nominating Hello Conversation Context issues for 40, as the amount of issues makes the feature questionable for release with this train.
This does not seem to be a FF40 release blocker. Tracking for FF41 instead.
Mark, I noticed that this bug is marked tracking for FF41. Is there a patch in the works on this one? Just trying to check whether I need to wontfix for FF41 and track for 42 instead. Thanks.
Flags: needinfo?(standard8)
Blocks: 1199120
This fixes it the issue by forcing the checkbox label to be a single line, and reducing the height of the comments field.

I decided to add the "useEllipsis" as I didn't want the tooltip on it unnecessarily on the panel view.

I've already agreed this with Sevaan over irc.
Attachment #8653398 - Flags: review?(mdeboer)
(In reply to Ritu Kothari (:ritu) from comment #3)
> Mark, I noticed that this bug is marked tracking for FF41. Is there a patch
> in the works on this one? Just trying to check whether I need to wontfix for
> FF41 and track for 42 instead. Thanks.

Patch attached, I think its simple enough to include in FF 41. I'll check if it needs any bitrot fixes for branches once the patch lands on truck.
Assignee: nobody → standard8
Iteration: --- → 43.2 - Sep 7
Points: --- → 3
Rank: 12
Flags: needinfo?(standard8)
Priority: -- → P1
Comment on attachment 8653398 [details] [diff] [review]
Fix unnecessary scroll on Windows while editing context.

Review of attachment 8653398 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8653398 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/9b640a52dca2
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Bogdan, would you be able to verify the fix on the latest Nightly? I think today's nightly should have the fix since the patch landed this morning PST.
Flags: needinfo?(bogdan.maris)
Ok, I just realised I forgot to check with a shorter title, and the text is now centred, rather than left-aligned. I'll get a new patch up to fix that.
Status: RESOLVED → REOPENED
Flags: needinfo?(bogdan.maris)
Resolution: FIXED → ---
Comment on attachment 8654006 [details] [diff] [review]
Follow-up to bug 1174702 - correctly align the title of the web page for the checkbox in the context editing.

Review of attachment 8654006 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM but we should also fix the vertical alignment of that text+checkbox.
Attachment #8654006 - Flags: review?(andrei.br92) → review+
(In reply to Andrei Oprea [:andreio] from comment #12)
> LGTM but we should also fix the vertical alignment of that text+checkbox.

On 41 this should be too much of an issue, for 42/43 I'm expecting bug 1190738 to fix it.
Comment on attachment 8653398 [details] [diff] [review]
Fix unnecessary scroll on Windows while editing context.

Approval Request Comment
[Feature/regressing bug #]: Context in conversations for Hello
[User impact if declined]: Scroll bars on Windows
[Describe test coverage new/current, TreeHerder]: Landed in m-c, has test coverage for the changes where appropraite
[Risks and why]: Low, small adjustments to layout, specifically the conversation window.
[String/UUID change made/needed]: None

The second patch is also needed.

I'm requesting this now as I'm out on Monday.
Attachment #8653398 - Flags: approval-mozilla-beta?
Attachment #8653398 - Flags: approval-mozilla-aurora?
Comment on attachment 8654006 [details] [diff] [review]
Follow-up to bug 1174702 - correctly align the title of the web page for the checkbox in the context editing.

Approval Request Comment
[Feature/regressing bug #]: Context in conversations for Hello
[User impact if declined]: The title text is centred rather than left-aligned.
[Describe test coverage new/current, TreeHerder]: Landed in fx-team, css-only
[Risks and why]: Low, css-only. Fixes a missing part of the previous patch.
[String/UUID change made/needed]: None
Attachment #8654006 - Flags: approval-mozilla-beta?
Attachment #8654006 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4e76075fa405
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Bogdan, would you be able to verify the fix on the latest Nightly build? This would give me more confidence to uplift this patch to beta. Thanks.
Flags: needinfo?(bogdan.maris)
Attached image 41Beta5_screenshot.png
(In reply to Ritu Kothari (:ritu) from comment #18)
> Bogdan, would you be able to verify the fix on the latest Nightly build?
> This would give me more confidence to uplift this patch to beta. Thanks.

Since Bogdan is on PTO for the next 2 weeks I've had a look at this today. I would recommend against uplifting this for two reasons:
1. The issue is now gone in Firefox 41 Beta 5 (Win 7 x64, Mac OS X 10.8.5, Ubuntu 12.04 x86) after the video area of the conversation window was increased by ~50 pixels - this allows the content of the Edit Context view to properly fit vertically now - the Edit Context view looks good now on all Operating Systems (see attached screenshot)
2. I can still see some UI issues in Aurora and Nightly (e.g. the Title checkbox looks much bigger than the Title text) so I'm worried that we may introduce some UI regressions if we uplift

Given the above I would prefer we let this ride the trains (or uplift to Aurora only) so we give Bogdan the chance to have a more thorough look at it and file any issues that may exist.
Comment on attachment 8653398 [details] [diff] [review]
Fix unnecessary scroll on Windows while editing context.

I agree with Florin's assessment and appreciate his quick follow up. Given that this is not a bug with a hugely negative end-user impact, I prefer letting this fix ride the trains while QE team (Bogdan) gives this more thorough testing to find and fix the issues together in Nightly.
Attachment #8653398 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8654006 [details] [diff] [review]
Follow-up to bug 1174702 - correctly align the title of the web page for the checkbox in the context editing.

Please see my comment on the other patch.
Attachment #8654006 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8653398 [details] [diff] [review]
Fix unnecessary scroll on Windows while editing context.

Please see my previous comment. It is also applicable for Aurora uplift.
Attachment #8653398 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8654006 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #19)
> Since Bogdan is on PTO for the next 2 weeks I've had a look at this today. I
> would recommend against uplifting this for two reasons:
> 1. The issue is now gone in Firefox 41 Beta 5 (Win 7 x64, Mac OS X 10.8.5,
> Ubuntu 12.04 x86) after the video area of the conversation window was
> increased by ~50 pixels - this allows the content of the Edit Context view
> to properly fit vertically now - the Edit Context view looks good now on all
> Operating Systems (see attached screenshot)

The test case here isn't the same. The screenshot shows the room was created with context, and then edited. If you create the room without context, then try to add some, you'll get the scrolling case.

> 2. I can still see some UI issues in Aurora and Nightly (e.g. the Title
> checkbox looks much bigger than the Title text) so I'm worried that we may
> introduce some UI regressions if we uplift

That's a separate issue tracked by bug 1190738. It shouldn't get in the way of uplifting this, as this is just changing the title text onto one line, and making a different box a bit shorter.
Flags: needinfo?(bogdan.maris) → needinfo?(rkothari)
Mark, thanks for your follow up comment. I am still not comfortable approving the first patch, the one that fixes the unnecessary scroll on editing. The patch seems too big and therefore increases the chances of regressions. 

I prefer approving the second patch about the title alignment as it's much smaller and therefore seems to be safe to uplift. Hope you are ok with this plan.
Flags: needinfo?(rkothari)
Comment on attachment 8654006 [details] [diff] [review]
Follow-up to bug 1174702 - correctly align the title of the web page for the checkbox in the context editing.

This patch seems simple and safe to uplift to Aurora42 and Beta41.
Attachment #8654006 - Flags: approval-mozilla-beta-
Attachment #8654006 - Flags: approval-mozilla-beta+
Attachment #8654006 - Flags: approval-mozilla-aurora-
Attachment #8654006 - Flags: approval-mozilla-aurora+
Comment on attachment 8654006 [details] [diff] [review]
Follow-up to bug 1174702 - correctly align the title of the web page for the checkbox in the context editing.

The second patch was a follow-up to the first, it isn't useful to land by itself.
Attachment #8654006 - Flags: approval-mozilla-beta+
Attachment #8654006 - Flags: approval-mozilla-aurora+
As noted in my comments above, I believe that this bug is not a release blocker and it would be best for the fix to ride the trains. We are about a week away from building 41 RC and therefore wontfix'ing.
Duplicate of this bug: 1172915
Mark, this bug (because of the followup patch) is a bit confusing. Did we fix it in 42? Thanks
Flags: needinfo?(standard8)
(In reply to Sylvestre Ledru [:sylvestre] from comment #29)
> Mark, this bug (because of the followup patch) is a bit confusing. Did we
> fix it in 42? Thanks

The patches work together. Given the first patch was denied for both 42 & 41 I didn't proceed further. If we do want to take them both for 42, then I think it would be possible.
Flags: needinfo?(standard8)
Comment on attachment 8654006 [details] [diff] [review]
Follow-up to bug 1174702 - correctly align the title of the web page for the checkbox in the context editing.

[Triage Comment]
OK, I am going to take them for 42. They should be in 42 beta 6, they have been in 43&43 for a while now without obvious regressions.
Attachment #8654006 - Flags: approval-mozilla-beta+
Attachment #8653398 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.