Adding header in theme messes up topic progress bar (when docked)

Product Support

AI-generated summary

A user, Lew_Grothe, reported an issue with the topic progress bar on mobile when using a theme with a header. The problem occurs when the #topic-progress-wrapper div changes from fixed to absolute and the bottom property is calculated incorrectly.

Lew_Grothe inspected the code and found that the calculation is based on the #main selector, which does not include the header. They suggested changing the calculation to use the body selector instead.

Lew_Grothe later provided more code and confirmed that changing the calculation to use $("body").height() fixes the issue. They also proposed an alternative solution using a wrapper div for the theme header.

pmusaraj asked if the instance was public to inspect the issue, but Lew_Grothe offered a private test site to help debug the problem.

pmusaraj confirmed the issue and agreed that targeting the body element makes sense. A pull request (PR) was prepared to fix the issue.

The PR was later merged, as announced by zogstrip.

1

When we added a header in our theme, on mobile, the toggle where the #topic-progress-wrapper div position is toggled from fixed to absolute, the bottom property is set wrong (doesn’t include the header when calculating the body length).

A little inspection and a posting from @Steven from 2016 provided enough of a solution but I hate leaving CSS cruft like this in our theme.

Seems like wherever the bottom property is calculated dynamically should be able to figure out the correct body length (including header)?

I hate to file this as a bug if I’m just missing something…

2

Can you provide code specifics, maybe @Johani can advise?

3

I’ll try - still trying to figure it out as my time for this permits…

All this is inspected on mobile view only…

When reading any topic which is either short or when scrolled to the bottom, the block:

<div id="topic-progress-wrapper" class="ember-view" style="right: 1em;"> 
...
</div>

is changed to

<div id="topic-progress-wrapper" class="docked ember-view" style="right: 1em; bottom: 82.4988px;">
...
</div>

Obviously, the bottom: property is calculated and the .docked class changes the position property from fixed to absolute. The bottom property seems to be calculated in (in the ember component discourse/components/topic-progress pretty-printed from minified in inspector):

        _dock: function() {
            var e = this.$();
            if (e && 0 !== e.length) {
                var t = window.pageYOffset || $("html").scrollTop()
                  , n = this.site.mobileView ? 0 : $("#topic-progress").height()
                  , i = $("#topic-bottom").offset().top + n
                  , o = $(window).height()
                  , s = $("#reply-control").height() || 0
                  , a = t >= i - o + s
                  , r = $("#main").height() - i;
                s > 0 ? e.css("bottom", a ? r : s) : e.css("bottom", a ? r : ""),
                this.set("docked", a);
                var l = $("#reply-control .reply-area");
                l && l.length > 0 ? e.css("right", l.offset().left + "px") : e.css("right", "1em")
            }
        },

Since this calculates the bottom property based on the window height of the #main selector, it does not include our header and, hence, the bottom property locates the box offset from the desired location.

Could bottom be calculated from the body.docked selector in this case instead?

(that’s all I have time for today…)

4

So, the code in question is:

  _dock() {
    const $wrapper = this.$();
    if (!$wrapper || $wrapper.length === 0) return;

    const offset = window.pageYOffset || $("html").scrollTop();
    const progressHeight = this.site.mobileView
      ? 0
      : $("#topic-progress").height();
    const maximumOffset = $("#topic-bottom").offset().top + progressHeight;
    const windowHeight = $(window).height();
    const composerHeight = $("#reply-control").height() || 0;
    const isDocked = offset >= maximumOffset - windowHeight + composerHeight;
    const bottom = $("#main").height() - maximumOffset;

    if (composerHeight > 0) {
      $wrapper.css("bottom", isDocked ? bottom : composerHeight);
    } else {
      $wrapper.css("bottom", isDocked ? bottom : "");
    }

    this.set("docked", isDocked);

    const $replyArea = $("#reply-control .reply-area");
    if ($replyArea && $replyArea.length > 0) {
      $wrapper.css("right", `${$replyArea.offset().left}px`);
    } else {
      $wrapper.css("right", "1em");
    }
  },

I have proven to myself that, in my case, changing

   const bottom = $("#main").height() - maximumOffset;

to

   const bottom = $("body").height() - maximumOffset;

works perfectly for our site.

I can imagine that you may want to not presume the body element selector would work in all cases so a perhaps more elegant solution would be to wrap the theme (brand?) header in a div that could be selected such that you could replace the bottom calculation with something like:

    const bottom = ((($("#brand_header").height() + $("#main").height()) - maximumOffset;

Suggestions? Where can we go from here?

5

Is your instance public? I tried to follow your code description, but this would be easier to track down if I could inspect your site. (For example, I added this component to a test site and couldn’t find any issues with the progress bar on mobile.)

6

It’s not public but I will PM you a login on our test site and pull all our special code out of the theme…

7

Thanks @Lew_Grothe, I do see the issue now. There are two lines of code that cause this when using a theme header, first:

const maximumOffset = $("#topic-bottom").offset().top + progressHeight;

offset.top here gets the element’s position from the top of the document. Later in the code, the position of the progress bar is calculated based on the height of #main minus said offset:

const bottom = $("#main").height() - maximumOffset;

But when adding an element to the body before #main, the offset will be misaligned with #main. I will prepare a PR tomorrow and propose to target the body element, as you suggest, it makes sense to me.

8

Any guess as to what release this might be in?

9

Whenever this PR is merged :wink:

https://github.com/discourse/discourse/pull/6425

EDIT: this PR is now merged