Closed Bug 1557139 Opened 2 years ago Closed 2 years ago

Better demarcation for sticky top header

Categories

(Webtools :: Searchfox, defect)

Product:
Webtools
Component:
Searchfox
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mccr8, Assigned: asuth)

References

Details

Attachments

(1 file)

screen shot of some suggested styling
38.68 KB, image/png
Details

The new sticky top header is super cool, but it can be hard to distinguish the context that it is showing from the actual code if things are lined up just so. It would be nice if there was a faint line or something after the context. I guess the slight gray background is supposed to do that, but it isn't quite distinct enough for my eyes.

Oh, and there's also a gray background for the contexts even when they are in the normal code. I didn't even see that until I was looking over things just now...

Agreed that we can do better. To journal my attempts for context/next steps:

  • I started with just background: white. This looked really freaky because there was no delineation.
  • I tried the gray background. This helped a lot.
  • I looked into what it would take to make the "position: sticky" bits look different when "stuck" versus when just part of the code.
    • Layout people don't like infinite loops, so there's no selector magic like "::hover" to allow for different styling when stuck.
    • I made a hack where we create a "doppleganger" that has the normal text presentation and is not position:sticky that overlays the actual position:sticky element. By way of CSS example where each copy got one of these:
.scope-dopple {
  z-index: 1000;
  background-color: white;
  position: absolute; /* these things are inside a position: relative containing block already */
  top: 0;
}
.scope-definer-1 {
  position: sticky;
  top: 72px;
  background-color: #eee;
  z-index: 999;
}
  • That created a very glitchy visual display, but the bigger problem was that if you selected text, you'd end up with two copies of the text. There's no CSS to take the content out of the selection region. (You can use user-select: none; to avoid letting a selection start/end in that range, but it can still include the text if the range starts outside it.) One could do an ugly hack using generated content though, as we'll need to do if we overhaul the line number display. See https://bugzilla.mozilla.org/show_bug.cgi?id=1511275#c1.
  • I think any kind of pure CSS approach here is likely to result in some kind of overpaint or general performance issue that makes layout/graphics teams sad.
  • This probably leaves using JS to handle things. I guess using the intersection observer API is probably the best way to avoid janking scrolling or being very inefficient with polling (https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API). We can add an extra CSS class that we set on the things that are stuck. It might want to animate fade-in/fade-out style because the timing in the threshold case might not be perfect.

Alternately, we could ditch position:sticky entirely and just use JS to manage things position:fixed style, but that felt less webby and using existing libs may entail making scrolling janky.

As an aside, I was thinking a next nice step is to change-up line numbering as in bug 1511275 which will allow us to have the line number stick to the line too. This would provide both a nice UX affordance to jump back to the line in question, but also make it less confusing since the sticky line would no longer potentially be lining up with someone else's line number.

I think for the purposes of this bug just making the gray more noticeable (maybe making it darker) should probably be sufficient. :mccr8, maybe experiment with devtools to find a color that works better for you?

Duplicate of this bug: 1557714

Yeah, all I'm really looking for is a darker background color. I'll see if I can get a chance to pick a darker color. Or maybe Olli could pick something.

Right now the main issue is that same background color is used for stuff at the top and for things which are being scrolled. That is making the UI confusing to my eyes. Could we have small border or darker bg when stuff is at the top.
Somehow it should be clear that the line numbers don't match to the top header. Hmm, perhaps the top header should cover line numbers?

The bg color is sufficiently dark/different on my display that it's not a problem. Can you use the devtools to pick a color that works for you? I'm happy to land a CSS change if you give me a color that works for you.

Here's a Google Intersection Observer demo/hack using Intersection Observer if anyone wants to get fancy:
https://developers.google.com/web/updates/2017/09/sticky-headers

In the meantime, I've taken bug 1511275 so we can make the line numbers stick with the line itself.

I implemented a "scroll" event sticky thing as part of my work in bug 1511275. Please check out the current prototype at https://dev.searchfox.org/ which has much more pronounced styling for stuck stuff.

The biggest problem I have with this header is that the line number to the left of it is the line number of the code that it obscures. And it generally lines up with it pretty exactly. It would help a lot if it were the line number that the floating line actually appears at.

(also maybe some kind of faint line below the floating portion to more clearly differentiate it from the code below...)

(In reply to Kris Maglione [:kmag] from comment #10)

The biggest problem I have with this header is that the line number to the left of it is the line number of the code that it obscures. And it generally lines up with it pretty exactly. It would help a lot if it were the line number that the floating line actually appears at.

(also maybe some kind of faint line below the floating portion to more clearly differentiate it from the code below...)

I think your comment applies to the currently-deployed version of the sticky header (i.e. bug 1533802) rather than this bug, which is (among other things) fixing the issues you pointed out.

The latest changes have finished baking and are available on https://searchfox.org/. Because of the current caching setup, there's a real chance you might need to hit ctrl-r twice or until the display converges :).

The current appearance of the styling is very arbitrary, so I'm needinfo-ing y'all to weigh in. If everyone's happy, I'll mark this bug fixed. Otherwise, we can make changes here, but I would request that people propose very specific CSS changes that they've exported from devtools.

Note that if we do fix this bug, the implementation of the styling will potentially still be enhanced in a follow-up. See the thread at https://github.com/mozsearch/mozsearch/pull/221#discussion_r292246209 but to summarize:

  • I used a linear-gradient hack to get the bottom bar going.
  • Doing things more elegantly seemed to run into line-drawing issues and rounding when dealing with situations where one CSS pixel does not equal one device pixel.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(continuation)
Flags: needinfo?(bugs)

Looks great to me. I'm able to easily tell the header apart now. Thanks.

Flags: needinfo?(continuation)

Yes, much better. Thanks!

Flags: needinfo?(bugs)

I took a couple of minutes to see if I could come up with some styling that is less blue. This is what I tried:

.source-line-with-number.stuck {
  background: #ffe;
}
.source-line-with-number.last-stuck {
  box-shadow: 0px 4px 8px #0002;
}

and then removing the .last-stuck div .c2 rule. See the attached screen shot. That's enough distinction for me, but might not be clear enough for others. Feel free to use that, or iterate on it.

I do like the shadow effect in :heycam's screenshot. :smaug and :mccr8 WDYT of the screenshot above, is that still ok for you to distinguish?

Flags: needinfo?(continuation)
Flags: needinfo?(bugs)

Sure, that's fine.

Flags: needinfo?(continuation)

(In reply to Cameron McCormack (:heycam) from comment #15)

Created attachment 9071490 [details]
screen shot of some suggested styling

I took a couple of minutes to see if I could come up with some styling that is less blue. This is what I tried:

This looks fantastic. It makes it obvious what's header and what isn't without making it hard to read either the lines in the header or the lines below it.

Flags: needinfo?(kmaglione+bmo)

Looking at the CSS I guess we probably want to adjust the colors slightly so that if a sticky header line is also highlighted it's easy to distinguish from a non-highlighted sticky header line. The highlighted one has a bgcolor of rgb(255,255,204) which is pretty close to #ffe.

I put :heycam's proposal in https://github.com/mozsearch/mozsearch/pull/229 and stood it up at https://dev.searchfox.org/
Please A/B test and we can also iterate on it in #searchfox.

(Uh, and to be clear, when I say A/B test, I really just mean test. I think the blue strawman draws way too much attention to the sticky stuff and :heycam's proposal is a massive improvement. As much as I think we should celebrate name-spaces, they don't really need to be the most prominent thing on the page. And the blue really resulted in contrast problems with the grey line numbers.)

Thanks. It looks pretty good to me, I have no real suggestions for improvement.

Assignee: nobody → bugmail

Assuming the yellow background is on top only, and not as method name background all the time, then looks great.

Flags: needinfo?(bugs)

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #20)

I put :heycam's proposal in https://github.com/mozsearch/mozsearch/pull/229 and stood it up at https://dev.searchfox.org/

Looks great to me!

I don't think there's anything else to do here, closing this out.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.