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

(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 on attachment 561102 [details] [diff] [review] unread - fix Looking into a minor change. Modified version should be up in a few hours.

A little cleaner and better fitting into the latest code change

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

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

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

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

Comments deleted, typo corrected

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.

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

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

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

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

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

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.

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

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.

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

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

Fix on old position

Alternative version; moved down to use existing if

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.

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

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

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

Checkin to mozilla-central needed Additional approval for mozilla-aurora requested

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

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

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

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.