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

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

Hi Evan and Ricky, The spec is updated. Thanks!

Here's the link of the spec: https://mozilla.invisionapp.com/share/ZDAGPK3AF#/218928209_5-1_General

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

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.

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.

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.

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 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

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 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.

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.

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?

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?

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.

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.

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.

(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.

Patch has updated for making search field sticky only.

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.

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

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 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.

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.

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 on attachment 8885631 [details] Bug 1377009 - Search field should always float on the top Please check comment 35.

Ah, sorry for the typo. Please check comment 34 + comment 35.

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.

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.

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

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.

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 :)