Search field should always float on the top

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
2 months ago
15 days ago

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

(Blocks: 1 bug)

56 Branch
Firefox 56
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 months ago
Search input field should always float on the top of preferences pane.

Updated

2 months ago
Assignee: nobody → evan
Status: NEW → ASSIGNED
Priority: P2 → P1

Comment 1

2 months ago
Hi Tina,

Once you update the spec, please let us know here.

Thank you.
Flags: needinfo?(thsieh)

Updated

2 months ago
Flags: qe-verify+
QA Contact: hani.yacoub

Updated

2 months ago
Whiteboard: [photon-preference][triage] → [photon-preference]
Hi Evan and Ricky,
The spec is updated. Thanks!
Flags: needinfo?(thsieh)
Here's the link of the spec: https://mozilla.invisionapp.com/share/ZDAGPK3AF#/218928209_5-1_General
(Assignee)

Updated

a month ago
Assignee: evan → rchien
(Assignee)

Comment 4

a month ago
Hey Helen, 

Can you provide the update visual spec for this? The spec in comment 3 doesn't provide details like there is a white background area behind search field which will always float on top wherever user scrolls and also the margin / padding / border style for white background itself.

Thanks
Flags: needinfo?(hhuang)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a month ago
Per discussion with Helen, we've had a conclusion about visual design for how search field will be floated on top.

* There will be no any border or specific border-bottom adding to floated hbox.float-content itself.
* Padding for hbox.float-content is `padding: 30px 0 40px 0`, so the space before the title like "General" will be increased as you see after applying the patch.

I'm going to remove Helen's ni flag since we've had above conclusion and there won't be a specific visual spec for this feature.
Flags: needinfo?(hhuang)

Comment 7

a month ago
mozreview-review
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review161738

::: browser/components/preferences/in-content-new/preferences.xul:204
(Diff revision 1)
>      </keyset>
>  
>      <html:a class="help-button" target="_blank" aria-label="&helpButton2.label;">&helpButton2.label;</html:a>
>  
>      <vbox class="main-content" flex="1">
> -      <hbox pack="end">
> +      <hbox class="float-content" pack="end">

This name, float-content, shouldn't be used because it uses a term 'float' which carries different implications within CSS. You could use 'search-container' or 'search-box' if needed.

::: browser/themes/shared/incontentprefs/preferences.inc.css:627
(Diff revision 1)
> +.float-content {
> +  position: fixed;

Please use position:sticky instead of position:fixed, and then you should be able to remove the margin-top: 70px; on #mainPrefPane.

::: browser/themes/shared/incontentprefs/preferences.inc.css:632
(Diff revision 1)
> +.float-content {
> +  position: fixed;
> +  background-color: var(--in-content-page-background);
> +  max-width: var(--preferences-content-width);
> +  width: var(--preferences-content-width);
> +  text-align: end;

Why is this needed when pack=end is already on the hbox?

::: browser/themes/shared/incontentprefs/preferences.inc.css:639
(Diff revision 1)
> +  padding: 30px 0 40px 0;
> +  z-index: 3;
> +}
> +
> +#searchInput {
> +  text-align: start;

If you can remove text-align:end above then you can remove this too.
Attachment #8885631 - Flags: review?(jaws) → review-
(Assignee)

Comment 8

a month ago
mozreview-review-reply
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review161738

> Why is this needed when pack=end is already on the hbox?

The pack=end won't work after applying `position: fixed` and I've no idea :(. But `position: sticky` could be a good solution for that.
(Assignee)

Comment 9

a month ago
Created attachment 8886045 [details] [diff] [review]
sticky.diff

Jared, I'm struggling with using position: sticky instead of fixed but unfortunately it's still buggy and scroll back will not make search-container fixed at the top as we want.

Here is my WIP patch with position sticky.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a month ago
Comment on attachment 8886045 [details] [diff] [review]
sticky.diff

Finally I found out a workaround to fix XUL position: sticky issue through setting:

.main-content {
  display: block;
  ...
}

So that we can see the .search-container can always float on the `top: 0` wherever you scroll.

Patch has been updated so please take a look. Thanks
Attachment #8886045 - Attachment is obsolete: true
(Assignee)

Comment 12

a month ago
Note that I wasn't clear in comment 9. The issue I struggled that is likely a XUL related issue when it comes to position: sticky. I tried so many HTML examples online and they all work pretty well until I applied the same approach in XUL.

As comment 11 mentioned, using display: block can workaround this issue and I've verified layout + RWD also take effect as expected.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a month ago
mozreview-review
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review162202

This still has a very large blank space on the left side that wastes space as the content is scrolled.

Can you remove the <hbox class="search-container"> and get the search box on the same line as the header, then put both the search box and the header as position:sticky? You may need to set a background-color on the header so the preferences aren't visible behind it as it scrolls.
Attachment #8885631 - Flags: review?(jaws) → review-
(Assignee)

Comment 16

a month ago
IIRC, you're mentioning about changing the visual spec.

Per discussion with Helen, we don't want to move the first level header (e.g. General, Privacy & Security) to the top within .search-container since the first level header will be removed in Fx57. As a result, there will be no first level header in the future and only search field always float on the top panel, and a second level header for each section will be introduced in Fx57 spec instead.

We believe that current implementation makes us able to adopt Fx57 changes more easily and reduce the extra effort to remove header again.
Comment hidden (mozreview-request)
Hi Helen, do you have a spec for how these second level headers will look? I am curious how they will be different than the current first level headers we already have.

What should we do about the large blank area that will be covering part of the preferences page when the user is scrolling?
Flags: needinfo?(hhuang)
(Assignee)

Comment 19

a month ago
Search field + Header float on the top. https://drive.google.com/open?id=0B7aDdSOQagatNGs5b2Z5YTBKZEU

Jared, I'm investigating the above video example to make header float on the top like search field with `position: sticky`. As you may know, the current implementation is using the `display: block` workaround on search input's parent container. And that will cause so many unexpected layout issues in its child elements. It's no an issue for search field since there is only one prefpane element which needs to fix. Unfortunately, there will be many child elements causing layout issues that is not that easy to fix.

Do you have any through about how to fix this XUL issue + position?
Flags: needinfo?(jaws)
Can we use the display:-moz-box; and position:relative; for the parent, then position:absolute for the search box? You might need to change some of the markup to get this positioned correctly.

Can you then do position:sticky on the header with top:-70px; or whatever is necessary to get the header to be visually in-line with the search box?

The video is close to what we should do, but instead the header should be in the same row as the search field. The video still shows them in different rows.
Flags: needinfo?(jaws)

Comment 21

a month ago
mozreview-review
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review163818

This patch needs to be rebased. It doesn't apply on m-c tip and I tried to use the 'hg pull -r ...' command but when I update to the tip with this patch I get an error about missing string entities.

I think my latest comment should help though.

::: browser/components/preferences/in-content-new/tests/browser_security.js:48
(Diff revision 5)
> -    // scroll the checkbox into the viewport and click checkbox
> +    checkbox.click();
> -    checkbox.scrollIntoView();
> -    EventUtils.synthesizeMouseAtCenter(checkbox, {}, gBrowser.selectedBrowser.contentWindow);

Why these changes? I think you may have made a mistake while rebasing.
Attachment #8885631 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a month ago
mozreview-review-reply
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review163818

> Why these changes? I think you may have made a mistake while rebasing.

I have no idea but my patch indeed cauesing this test failure on try. This line changes can fix the test failure.
Sorry for the late reply, I'm still working on the visual spec for Fx57, and all the mockups I made is updated to InVision.

As Ricky mentioned above, the first level header will be removed in Fx57 but showing the second level header (section name) instead. Also, it's for sure that we want to keep the search field always float on top of the content in Fx56.

For the blank area on top of the first level header, which belongs to the search field, since we want to emphasize the search function in Fx57, the proper white space could help users differentiate it from the content area, and consider it as an overall search in Preferences not just for the individual page.
Flags: needinfo?(hhuang)
(Assignee)

Comment 26

a month ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Can we use the display:-moz-box; and position:relative; for the parent, then
> position:absolute for the search box? You might need to change some of the
> markup to get this positioned correctly.

Unfortunately, this approach doesn't work for me. position: absolute is unable to make search field container float on the top unless we set display: block for its parent instead of display: -moz-box. But it means that it goes back to comment 19 to cause child elements layout issues. 

> The video is close to what we should do, but instead the header should be in
> the same row as the search field. The video still shows them in different
> rows.

Here is an update about visual spec per discussion with designers. We're going to ship sticky search field in Fx56 and then there will be a new spec for sticky header in Fx57 (spec is under discussion). Visual team has decided and confirmed that the search field container will float on the top, although there is a blank space on the left side. As a result, this bug is targeting on Fx56 and implementing the sticky search field for Fx56.

If you have any question about sticky header for Fx57, you might have to add your comment on Invision or contact Helen for more details.
Comment hidden (mozreview-request)
(Assignee)

Comment 28

a month ago
Patch has updated for making search field sticky only.

Comment 29

a month ago
mozreview-review
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review164792

::: browser/themes/shared/incontentprefs/preferences.inc.css:654
(Diff revision 8)
>    color: var(--in-content-category-text);
>    text-decoration: none;
>  }
>  
> +.search-container {
> +  position: sticky;

Sorry for misleading you, but we should not be using position:sticky while we are still using XUL layout. See https://bugzilla.mozilla.org/show_bug.cgi?id=987339 (and note that it doesn't work correctly either).

::: browser/themes/shared/incontentprefs/preferences.inc.css:659
(Diff revision 8)
> +  position: sticky;
> +  background-color: var(--in-content-page-background);
> +  width: 100%;
> +  top: 0;
> +  padding: 30px 0 40px 0;
> +  z-index: 3;

z-index: 1;

I don't see anything that causes us to need to use 3 here.
Attachment #8885631 - Flags: review?(jaws) → review-
(Assignee)

Comment 30

a month ago
mozreview-review-reply
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review164792

> Sorry for misleading you, but we should not be using position:sticky while we are still using XUL layout. See https://bugzilla.mozilla.org/show_bug.cgi?id=987339 (and note that it doesn't work correctly either).

Thanks for pointing it out. I know it's a known issue so I used a workaround mechanism through setting parent `display: block` to fix the issue as comment 19 mentioned.

Did you see any sticky issue after applying my patch? (I can't spot any issue when using this workaround) Or do you think there are some risks we should avoid using `position: sticky`?

Thanks
Comment hidden (mozreview-request)
I did not see any issue but that is because your use of position:sticky is with an element that is already at the top of the page and therefore doesn't really benefit from how position:sticky works. It would work better for things like the subcategory headers.

We shouldn't use position:sticky here because it was never implemented for XUL and we may have bugs that are hard to fix or weird since it is unsupported.

Comment 33

a month ago
mozreview-review
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review165924

::: browser/components/preferences/in-content-new/tests/browser_security-2.js:45
(Diff revision 9)
> -    // scroll the checkbox into view, otherwise the synthesizeMouseAtCenter will be ignored, and click it
> +    checkbox.click();
> -    checkbox.scrollIntoView();
> -    EventUtils.synthesizeMouseAtCenter(checkbox, {}, gBrowser.selectedBrowser.contentWindow);

These test changes shouldn't be part of the patch. I assume if these get checked in this test will fail.

::: browser/themes/shared/incontentprefs/preferences.inc.css:660
(Diff revision 9)
>    color: var(--in-content-category-text);
>    text-decoration: none;
>  }
>  
> +.search-container {
> +  position: sticky;

Please change this to position:fixed.
Attachment #8885631 - Flags: review?(jaws) → review+
(Assignee)

Comment 34

a month ago
Jared,

Setting `position: sticky` for .search-container and setting `position: block` for its parent .pane-container could be a better solution IMO.

After applying `position: fixed` for .search-container, it takes entire .search-container out of layout flow and then cause resizing issue like .search-container itself won't shrink with #mainPrefPane panel accordingly unless we introduce window resize callback to adjust its width. I believe `position: sticky` is still really benefit since .search-container element will stay in layout flow so that can mitigate some layout or resize issues.

If that makes sense to you, I'm going to land the current `position: sticky` patch.
Flags: needinfo?(jaws)
(Assignee)

Comment 35

a month ago
mozreview-review-reply
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

https://reviewboard.mozilla.org/r/156478/#review165924

> These test changes shouldn't be part of the patch. I assume if these get checked in this test will fail.

I've reverted this changes and seen test still green.

I believe previous failures are caused by using `position: fixed` but it has been fixed after applying `position: sticky` duo to some layout flow issues.
Comment hidden (mozreview-request)
(Assignee)

Comment 37

a month ago
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

Please check comment 35.
Flags: needinfo?(jaws)
Attachment #8885631 - Flags: feedback?(jaws)
(Assignee)

Comment 38

a month ago
Ah, sorry for the typo. Please check comment 34 + comment 35.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

a month ago
Note that patch has updated in order to match the visual spec at https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683024

According to latest visual spec, the .search-container heigh will be 70px and margin-top and margin-bottom of #searchInput itself should be 20px.
(Assignee)

Updated

a month ago
Attachment #8885631 - Flags: feedback?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Updated

27 days ago
Attachment #8885631 - Flags: feedback?(jaws)
Comment on attachment 8885631 [details]
Bug 1377009 - Search field should always float on the top

Okay, we can push forward with this patch. I have played around with it for a bit now and I'll live with it.

@Tina, can we *please* do something about the bar of blank space on the left side of the search box? While the page is scrolling, the content goes behind this box and it looks odd how it is cut off. This could be handled in another bug of course.
Flags: needinfo?(thsieh)
Attachment #8885631 - Flags: feedback?(jaws) → feedback+

Comment 44

26 days ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/691b88d4102b
Search field should always float on the top r=jaws

Comment 45

25 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/691b88d4102b
Status: ASSIGNED → RESOLVED
Last Resolved: 25 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED

Comment 46

24 days ago
Build ID: 20170730100307
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Hi Jared,
We discussed the layout design with the entire Taipei visual team and believed that the space looks balanced on 57. I'll clear the needinfo here and look forward to the visual refresh on 57 :)
Flags: needinfo?(thsieh)
You need to log in before you can comment on or make changes to this bug.