687754 - [followup on bug 487242] unread attribute set at different times during page load

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Len]

Attachment: unread - fix

While the first tab gets the attribute after it finished loading the page, all other tabs are getting it far to early (at start of loading).

Attribute should not reset when in _previewMode.

Added patch to fix both

Comment 1

[ithinc]

Comment on attachment 561102 [details] [diff] [review]
unread - fix

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

::: browser/base/content/tabbrowser.xml
@@ +567,5 @@
>                    this.mTab.removeAttribute("busy");
>                    this.mTabBrowser._tabAttrModified(this.mTab);
>                  }
>                  this.mTab.removeAttribute("progress");
> +                if (!this.mTab.selected && !oldBlank)

Can "unread" state be set on DOMTitleChanged as well? DOMTitleChanged means another kind of "unread", doesn't it? Plus it will avoid the inconsistency between the first tab and other tabs.

@@ +877,5 @@
>              this.mCurrentBrowser = newBrowser;
>              this.mCurrentTab = this.selectedTab;
> +            // Remove 'unread' if not in Preview mode
> +            if (!this._previewMode)
> +              this.mCurrentTab.removeAttribute("unread");

Move it lower to be parallel to removeAttribute("titlechanged").

Comment 2

[Len]

(In reply to ithinc from comment #1)

'unread' is indicating a new/reloaded document in a tab that is not selected.
DOMTitleChanged doesn't necessary indicates a new/reloaded document and fires on selected tabs too.
Some JS in a document could for example change the title every few seconds.
You see the difference?

The attachment is fixing the inconsistency between the first and the following tabs. It was caused by the different handling of those tabs by the tabbrowser itself. Now all tabs get the unread attribute after they finished loading the document. A selected tab will not have that attribute.

Example of use of 'unread' in userChrome.css:
/* set color for unread (and not selected) tabs to blue */
.tabbrowser-tab[unread]
{ color: blue ! important; }

If you additionally want the same coloring while a document loads and if the title has changed, you can add that to the css selector. Watch out that 'busy' and 'titlechanged' both need the additional not(selected).

Comment 3

[Len]

Comment on attachment 561102 [details] [diff] [review]
unread - fix

Looking into a minor change.
Modified version should be up in a few hours.

Comment 4

[Len]

Attachment: unread - fix (new)

A little cleaner and better fitting into the latest code change

Comment 5

[ithinc]

(In reply to Len from comment #2)
> 'unread' is indicating a new/reloaded document in a tab that is not selected.
> DOMTitleChanged doesn't necessary indicates a new/reloaded document and
> fires on selected tabs too.
> Some JS in a document could for example change the title every few seconds.

If Gmail changes its title in background, I'm sure it should be marked as unread. If some JS changes the title indeed, it must indicate that there's something new, otherwise it could be a fault.

Comment 6

[Len]

- unread points to the document in the tab
- titleChanged points to the title of the document in the tab

If Gmail has reloaded the page, I agree and would wonder if it wouldn't be marked unread.
If Gmail only changes the title on an already read page, you can mark that easy with .tabbrowser-tab[titleChanged]
I wouldn't like to see a tab marked unread after I did read it, only because the page likes to circle through a couple of headlines in the title.

I prefer to differ between page title and the page itself. So you have the choice to mark the tab depending on which one you want and how. Give the user a chance to decide what he wants.

If you want both marked the same way, you can do it for example like this:
/* set color for titleChanged and unread (and not selected) tabs to blue */
.tabbrowser-tab[unread],
.tabbrowser-tab[titleChanged]
{ color: blue ! important; }

You can even mark the case of a changed title on an already read tab differently (i.e. red).
Or you could use the text color for one case and the background color for the other case.
The user has the choice.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 562201 [details] [diff] [review]
unread - fix (new)

>                 if (this.mTab.hasAttribute("busy")) {
...
>+                  // Only set unread when busy is removed
>+                  if (!this.mTab.selected) {
>+                    this.mTab.setAttribute("unread", "true");

>+            // Remove 'unread' if not in Preview mode
>+            if (!this._previewMode)
>+              this.mCurrentTab.removeAttribute("unread");

We don't need these comments, they just read out the code.

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/0eacb6c66396

Comment 9

[Dão Gottwald [:dao]]

Backed out since this contained a typo (mismatched brackets) that completely broke Firefox.

Comment 10

[Len]

(In reply to Dão Gottwald [:dao] from comment #9)
> Backed out since this contained a typo (mismatched brackets) that completely
> broke Firefox.

Sorry about that. Leftover from a test line when copying over.

Comment 11

[Len]

Attachment: unread - fix (corr)

Comments deleted, typo corrected

Comment 12

[ithinc]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#911

913             if (!this._previewMode) {
914 #ifdef MOZ_E10S_COMPAT
915               // Bug 666816 - TypeAheadFind support for e10s
916 #else
917               this._fastFind.setDocShell(this.mCurrentBrowser.docShell);
918 #endif
919 
920               this.updateTitlebar();
921 
922               this.mCurrentTab.removeAttribute("titlechanged");
923             }

It would be better to place removeAttribute("unread") here.

Comment 13

[Len]

(In reply to ithinc from comment #12)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#911
> 
> 913             if (!this._previewMode) {
> 914 #ifdef MOZ_E10S_COMPAT
> 915               // Bug 666816 - TypeAheadFind support for e10s
> 916 #else
> 917               this._fastFind.setDocShell(this.mCurrentBrowser.docShell);
> 918 #endif
> 919 
> 920               this.updateTitlebar();
> 921 
> 922               this.mCurrentTab.removeAttribute("titlechanged");
> 923             }
> 
> It would be better to place removeAttribute("unread") here.

I think I have answered that before in comment #6.
"titlechanged" /= "unread"
Basically "unread" says 'page is loaded from network in background' and removed as soon as the tab is selected.
It is not and should not been connected to the title. "titlechanged" has it's own logic when it is set and removed. You will have some overlapping but that is all. "titlechanged" might come without "unread", but it's nothing that "unread" has to worry about.
"unread" tells you if you have selected (unread/read) a tab after a page was loaded from the web.
"titlechanged" tells you if the title of a tab has changed (no matter what caused it).
To move removeAttribute("unread") down to where you suggested means, to put it after several ProcessListeners are started and than into or after the patch of bug 540248 (related to titlebar).
To remove unread right after selecting a tab (selected -> read) and than let the ProcessListeners get called seems the far better choice to me.

You have the chance to let both attributes lead to the same marking of the tab, like you want to. Others like me don't like that and can handle them separate or ignore "titlechanged".

Comment 14

[ithinc]

(In reply to Len from comment #13)
> To move removeAttribute("unread") down to where you suggested means, to put
> it after several ProcessListeners are started and than into or after the
> patch of bug 540248 (related to titlebar).
> To remove unread right after selecting a tab (selected -> read) and than let
> the ProcessListeners get called seems the far better choice to me.

I see no difference. My suggestion was just for code formatting, not for other purpose.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Len from comment #13)
> To remove unread right after selecting a tab (selected -> read) and than let
> the ProcessListeners get called seems the far better choice to me.

Why exactly would it matter whether the attribute got removed before or after calling these listeners?

Comment 16

[Len]

(In reply to Dão Gottwald [:dao] from comment #15)
> Why exactly would it matter whether the attribute got removed before or
> after calling these listeners?

a) Maybe switching lines
> this.mCurrentTab = this.selectedTab;
> if (!this._previewMode)
>   this.mCurrentTab.removeAttribute("unread");
> this.showTab(this.mCurrentTab);
to
> this.mCurrentTab = this.selectedTab;
> this.showTab(this.mCurrentTab);
> if (!this._previewMode)
>   this.mCurrentTab.removeAttribute("unread");
if that seems more convenient.
This would still keep it right were the tab gets selected and viewed, the reason why "unread" is getting removed. It's a clear chain of commands that are logically tight connected to each other. Like removing "busy" leads to setting "unread" (if not selected). Easy to understand and maintain.

b) I can't guarantee the Listener calls do not lead to (rare) cases where "unread" wouldn't be removed. Is there someone able to guarantee that for now and in the future? Why even take that risk? At this point in code the tab is already selected and shouldn't have the attribute anymore.

c) After those Listeners we have the code block that handles fastFind and TitleBar if not in _previewMode.
The only connection I can see here is _previewMode.

Does it really make sense to move it down 40+ lines to save a single 'if' and at the same time loosing the easy logical understanding of why it has to be removed (selected/viewed => read) plus wondering if those Listener calls might at some point cause something related to removing "unread" we haven't thought of right now.

If those arguments don't catch, we can even try to move it down another 30+ lines to the TabSelect events where the _previewMode is handled too and test it there.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Len from comment #16)
> This would still keep it right were the tab gets selected and viewed, the
> reason why "unread" is getting removed.

All of updateCurrentBrowser runs because the tab gets selected.

> b) I can't guarantee the Listener calls do not lead to (rare) cases where
> "unread" wouldn't be removed.

I don't see why it wouldn't be removed.

> c) After those Listeners we have the code block that handles fastFind and
> TitleBar if not in _previewMode.
> The only connection I can see here is _previewMode.

Right.

Comment 18

[Len]

So you want the attribute to be removed down where the tab select events are?

> // TabSelect events are suppressed during preview mode to avoid confusing extensions and other bits of code
> // that might rely upon the other changes suppressed.
> // Focus is suppressed in the event that the main browser window is minimized - focusing a tab would restore the window
> if (!this._previewMode) {
>   // We've selected the new tab, so go ahead and notify listeners.
>   let event = document.createEvent("Events");
>   event.initEvent("TabSelect", true, false);
>   this.mCurrentTab.dispatchEvent(event);
> 
>   this.mCurrentTab.removeAttribute("unread");
> 
>   this._tabAttrModified(oldTab);
>   this._tabAttrModified(this.mCurrentTab);

This would make more sense to me than the fastFind/TitleBar block.

In a few hours I could run some quick tests and upload the new patch.

Comment 19

[ithinc]

(In reply to Len from comment #18)
> So you want the attribute to be removed down where the tab select events are?

It is slightly different for add-ons to put it before or after the TabSelect event's dispatching.

Comment 20

[Dão Gottwald [:dao]]

The attribute removal should happen before the TabSelect event, I think. Other than that, it's not really correlated with the TabSelect event. As I said, everything in this method is about a tab being selected.

Comment 21

[Len]

(In reply to Dão Gottwald [:dao] from comment #20)
> Other than that, it's not really correlated with the TabSelect event. As I
> said, everything in this method is about a tab being selected.

fastFind and TitleBar are even less related. So using the 'if (!this._previewMode)' of the TabSelect block makes more sense if we want to move it down to save a separate 'if' for removing the attribute.

Anyway. To make some progress I am going to split the patch into 2 parts:
Part 1: Only set unread when busy is removed
Part 2: Don't remove unread in preview mode (fix on old position)
Part 2 alternate: Don't remove unread in preview mode (moved down to use existing if)

This way part 1 can get applied and you can choose (or we can discuss further) which one of part 2 is better.
After the fixes did land in mozilla-central, we should take care they are getting in aurora too before FF9 moves to beta.

Comment 22

[Len]

Attachment: Only set unread when busy is removed

Fixes the different times "unread" is set in tab 1 and tab 2+

Comment 23

[Len]

Attachment: Don't remove unread in preview mode

Fix on old position

Comment 24

[Len]

Attachment: Don't remove unread in preview mode (alternative version)

Alternative version; moved down to use existing if

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 565093 [details] [diff] [review]
Don't remove unread in preview mode (alternative version)

Please move this to the earlier !this._previewMode section. It doesn't matter that it doesn't correlate with the other pieces there, as those pieces don't correlate with each other either.

Comment 26

[Len]

Attachment: Don't remove unread in preview mode (next try)

Additionally moved leading comment into the 'if' to avoid confusion with that other patch.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 565359 [details] [diff] [review]
Don't remove unread in preview mode (next try)

>+              // Don't switch the fast find or update the titlebar (bug 540248) - this tab switch is temporary

just remove this comment

Comment 28

[Len]

Attachment: Don't remove unread in preview mode (final)

(In reply to Dão Gottwald [:dao] from comment #27)

> >+              // Don't switch the fast find or update the titlebar (bug 540248) - this tab switch is temporary
> 
> just remove this comment

Done.

Comment 29

[Len]

Checkin to mozilla-central needed

Additional approval for mozilla-aurora requested

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2561f2a8512c
http://hg.mozilla.org/integration/mozilla-inbound/rev/52bde8206312

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2561f2a8512c
https://hg.mozilla.org/mozilla-central/rev/52bde8206312

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 565090 [details] [diff] [review]
Only set unread when busy is removed

This feels like a bug with low user impact that can wait until 10 ships. If there's some large number of users having a really bad experience can you please explain that and re-nominate.

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 565397 [details] [diff] [review]
Don't remove unread in preview mode (final)

This feels like a bug with low user impact that can wait until 10 ships. If there's some large number of users having a really bad experience can you please explain that and re-nominate.

Comment 34

[Len]

The unread attribute got added to FF9 on 2011-09-15 (see Bug 487242) and I think it would be better to have it completely fixed before FF9 is getting released.
It's a no risk fix. Since this second patch only moves the 3 lines (add and remove attribute) of the first patch to a different position in the code, there is no reason why it should have any bad impact.
Seamonkey already released version 2.4 with the adapted patch from bug 487242 and there is a question from only 2 days ago if this second patch will be adapted too. I saw on userstyles.org the addition of "unread" in FF9 is noted there too. I think this shows the interest in having this old regression (bug 487242) finally fixed.