tab.previousSibling, tab.nextSibling, tabContainer.firstChild, tabContainer.lastChild are in use by Firefox code and many extension. when we have more then one group, this properties can return tab that is not visible. this is unexpected result. beforeselected, afterselected , first-tab, last-tab attribute can be wrong

Overriding the stock DOM properties doesn't seem sane to me. How about adding previousVisibleTab, nextVisibleTab, firstVisibleTab, lastVisibleTab? Could be without "Visible" even.

(In reply to comment #1) > Overriding the stock DOM properties doesn't seem sane to me. > How about adding previousVisibleTab, nextVisibleTab, firstVisibleTab, > lastVisibleTab? Could be without "Visible" even. that will be OK. what about beforeselected, afterselected , first-tab, last-tab attributes some themes use these attributes.

Adds previousTab/nextTab to tabs. Adds firstTab and lastTab to tabbrowser, and sets them as attributes. It also adds the following, which Australis tabs need available: 1. Adds before/after selected/hovered tab properties and sets them as attributes. 2. Lets the in-tab-strip new tab button receive the after* properties and attributes. 3. Adds property/attribute "beforeNewTabButton" for the last visible tab. Needed because the last tab needs to have an end separator in Australis iff the new tab button is in the tab strip after it. The implementation for each property added to tabbrowser is a private field storing the tab reference, with a property getter/setter to remove the old tab's attribute, add the new tab's attribute, and update the field's value.

This is using mouseenter/mouseleave for the hovered parts. A warning (from nsEventListenerManager.cpp:291) points out that those are slower than mouseover/mouseout, so alternatives are welcome.

This will get the new tab button by its id, instead of through DOM walking, which should be a bit cleaner/less error prone. Also cleaned up |markTabs| a bit to make sure that unused properties get properly cleared.

I applied the patch and noticed that I can often get the last tab to have the |selected| attribute as well as the |afterselected| attribute. If the tab is the first one after a single apptab, then the first tab can sometimes have |selected| as well as |beforeselected|.

r(In reply to Adam [:hobophobe] from comment #3) > 2. Lets the in-tab-strip new tab button receive the after* properties and > attributes. > 3. Adds property/attribute "beforeNewTabButton" for the last visible tab. > Needed because the last tab needs to have an end separator in Australis iff > the new tab button is in the tab strip after it. We don't need these. I know how to implement the Australis theming without this. Also, even though we don't need it, it's already possible to determine via pure CSS whether the in-tab-strip new tab button is immediately after the selected/:hover tab. The following should be sufficient: first-tab last-tab beforeselected afterselected beforehover afterhover

Comment on attachment 628441 [details] [diff] [review] Add id for new tab button, clean up a bit more. We really appreciate you working on this and creating a patch. Thanks. :) This already works correctly most of the time, but Jared's and my comments need to be addressed.

Thanks Jared and Frank. This addresses the deficiencies in the previous patch. This changes when |markTabs| gets called in |addTab| as tabs could be moved around up to very late in that. Calls |markTabs| via |updateCurrentBrowser|, to always re-mark when selectedTab changes. It updates the setters for beforeselected/afterselected to remove beforehovered/afterhovered (so when a hovered tab gets selected, the hovered attributes get removed from its neighbors). Also updates beforehovered/afterhovered setters to explicitly avoid setting to them on the selected tab. Adds a few more tests for after pinning a tab.

Comment on attachment 629431 [details] [diff] [review] Remove beforeNewTabButton, don't set attributes on new tab button. > <property name="visibleTabs" readonly="true"> > <getter><![CDATA[ >- if (!this._visibleTabs) >+ if (!this._visibleTabs) { > this._visibleTabs = Array.filter(this.tabs, > function (tab) !tab.hidden && !tab.closing); >+ this.markTabs(); >+ } > return this._visibleTabs; > ]]></getter> > </property> Putting this in a getter that may or may not run when necessary doesn't make sense. >+ <method name="markTabs"> It probably makes more sense to implement this in the tabbrowser-tabs binding. Also prefix it with an underscore. >+ <property name="firstTab"> >+ <property name="lastTab"> >+ <property name="beforeSelectedTab"> >+ <property name="afterSelectedTab"> >+ <property name="beforeHoveredTab"> >+ <property name="afterHoveredTab"> Don't expose these properties. All the getters are unused. >+ <method name="_setBeforeHovered"> >+ <parameter name="aEvent"/> >+ <body><![CDATA[ >+ let vTabs = gBrowser.visibleTabs; >+ this.beforeHoveredTab = vTabs[vTabs.length - 1]; >+ ]]></body> >+ </method> >+ >+ <method name="_clearBeforeHovered"> >+ <parameter name="aEvent"/> >+ <body><![CDATA[ >+ this.beforeHoveredTab = null; >+ ]]></body> >+ </method> These methods seem to be tailored for the new tab button. Their names should make this clear. aEvent is unused. >+ <property name="previousTab" readonly="true"> >+ <property name="nextTab" readonly="true"> Remove these from this patch. They're unrelated to the rest. >+ <handler event="mouseenter"><![CDATA[ >+ if (event.originalTarget == event.currentTarget && >+ event.currentTarget.classList.contains("tabbrowser-tab")) { mouseenter and mouseleave don't bubble. You can't remove the tabbox.xml code. It's in use.

Thanks, Dão. The tests now call |_markTabs| directly after calls to |showTab| and |hideTab|, as those don't trigger it. The non-test consumers of those do trigger |_markTabs|. To avoid conflicts with the attributes from tabbox, these are the new attributes: first-visible-tab last-visible-tab beforeselected-visible afterselected-visible

Sending the patch to the tryserver to see if it causes any test failures.

ok I've tried the patch on UX, what about the pinned tabs??? First, pin more than 3 tabs, then make the tab-bar over-flowed. Select an unpinned tab, hover those pinned tab and you'll see the tabs are 1px overlap the nav-bar. Next select the 1st pinned tab, then hover the 2nd and the 3rd pinned tabs, you'll see the unselected tabs background overlapping the selected tabs background. I hope this will be fixed in the next patch.

I manually pushed this to try server. Can you check to make sure that all tests are passing? https://tbpl.mozilla.org/?tree=Try&rev=6d4643272944 (In reply to Nguyen Bat Hung from comment #14) > ok I've tried the patch on UX, what about the pinned tabs??? Thanks for the feedback. Comments on the look and feel belong with bug 738491. :)

(In reply to Jared Wein [:jaws] from comment #15) > Thanks for the feedback. Comments on the look and feel belong with bug > 738491. :) Actually, the look and feel is ok, I thinks the problem belongs to 'afterhover tab attributes' for pinned tab when tab-bar is over-flowed. Have you tried to reproduce the problem yet???

The test failures were due to the case where the selectedTab mightn't be a member of the visibleTabs. This version will skip setting those attributes in that case and remove the attributes from any tabs with them. I wasn't able to recreate the failures for browser_bug406216.js here. I tried to recreate the pinned tab problem, but the nightly I downloaded (Linux) didn't yet have the Australis tabs. I did notice that in the described case the pinned tabs get extra negative margin (via style "-moz-margin-start"). That happens from tabbrowser.xml:~3140, |_positionPinnedTabs|. I looked at the hover attributes on the pinned tabs in that case, and they seemed to be set correctly.

Pushed the latest patch to try server: https://tbpl.mozilla.org/?tree=Try&rev=e02d37cc5987

Comment on attachment 630862 [details] [diff] [review] Don't mark selection-related attributes if the selected tab is not in the visibleTabs. Review of attachment 630862 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating your patch. :) > Don't mark selection-related attributes if the selected tab is not in the visibleTabs. In which cases is the selected tab not among the visibleTabs? I don't think we ever intentionally do that. ::: browser/base/content/tabbrowser.xml @@ +2814,5 @@ > </field> > > + <field name="_firstTab"> > + null > + </field> My personal preference is that each of these simply take up one line, i.e.: <field name="_firstTab">null</field> <field name="_lastTab">null</field> ... @@ +2838,5 @@ > + </field> > + > + <method name="_markTabs"> > + <body><![CDATA[ > + let vTabs = this.tabbrowser.visibleTabs; For consistency's sake, could you use `visibleTabs` instead of `vTabs`? @@ +2861,5 @@ > + } > + > + if (this._afterSelectedTab) > + this._afterSelectedTab.removeAttribute("afterselected-visible"); > + if (lastVisible == selectedIndex || selectedIndex == -1) { Nit: Use `if (selectedIndex == lastVisible || selectedIndex == -1)` for consistency. @@ +2874,5 @@ > + > + if (this._firstTab) > + this._firstTab.removeAttribute("first-visible-tab"); > + this._firstTab = vTabs[0]; > + this._firstTab.setAttribute("first-visible-tab", "true"); I'm unhappy with the long, slightly inconsistent attribute names that also don't quite match their corresponding property names, but I guess it can't be helped much. @@ +2934,5 @@ > </property> > > + <method name="_enterNewTab"> > + <body><![CDATA[ > + let candidate = gBrowser.visibleTabs[gBrowser.visibleTabs.length - 1]; let visibleTabs = this.tabbrowser.visibleTabs; let candidate = visibleTabs[visibleTabs.length - 1]; @@ +2935,5 @@ > > + <method name="_enterNewTab"> > + <body><![CDATA[ > + let candidate = gBrowser.visibleTabs[gBrowser.visibleTabs.length - 1]; > + if (!candidate.hasAttribute("selected")) { Due to old add-on code possibly setting selected="false" and to maintain consistency with our stylesheet selectors, .hasAttribute("selected") isn't enough. Replace all instances of .hasAttribute("selected") with .selected, please. @@ +3912,5 @@ > </handler> > + <handler event="mouseenter"><![CDATA[ > + let tab = event.target; > + if (tab.hasAttribute("selected") || > + event.originalTarget.localName != "tab") We should be consistent here. Either both should be event.target or both should be event.originalTarget. Likewise for mouseleave. @@ +3915,5 @@ > + if (tab.hasAttribute("selected") || > + event.originalTarget.localName != "tab") > + return; > + > + let visibleTabs = gBrowser.visibleTabs; let visibleTabs = this.tabbrowser.visibleTabs; @@ +3917,5 @@ > + return; > + > + let visibleTabs = gBrowser.visibleTabs; > + let tabIndex = visibleTabs.indexOf(tab); > + let tabcontainer = gBrowser.mTabContainer; let tabContainer = this.parentNode; @@ +3923,5 @@ > + tabcontainer._beforeHoveredTab = null; > + } else { > + let candidate = visibleTabs[tabIndex - 1]; > + if (!candidate.hasAttribute("selected")) { > + tabcontainer._beforeHoveredTab = candidate Nit: Semicolon; @@ +3928,5 @@ > + candidate.setAttribute("beforehovered", "true"); > + } > + } > + > + if (visibleTabs.length - 1 == tabIndex) { Nit: Use `tabIndex == visibleTabs.length - 1` to match the previous if condition. @@ +3941,5 @@ > + ]]></handler> > + <handler event="mouseleave"><![CDATA[ > + let tabcontainer = gBrowser.mTabContainer; > + if (event.originalTarget.localName != "tab") > + return; if (event.originalTarget.localName != "tab") return; let tabContainer = this.parentNode;

(In reply to Frank Yan (:fryn) from comment #19) > We should be consistent here. Either both should be event.target or both > should be event.originalTarget. Likewise for mouseleave. You only need originalTarget when you care about anonymous content.

> > Don't mark selection-related attributes if the selected tab is not in the visibleTabs. > > In which cases is the selected tab not among the visibleTabs? > I don't think we ever intentionally do that. This was occurring during the test for bug 455852 on the try builds. That tests closing the only tab when browser.tabs.closeWindowWithLastTab is false. |_markTabs| gets triggered by adding a new tab to replace the only, closing tab. But that tab is already marked |closing| (excluding it from |visibleTabs|), yet still the |selectedTab|. This is a brief state, as the new tab gets selected and the tabs get remarked once the old tab is done being removed.

Comment on attachment 633636 [details] [diff] [review] Updated patch addressing feedback. Review of attachment 633636 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +2826,5 @@ > + > + if (!visibleTabs.length) { > + return; > + } > + let selectedIndex = visibleTabs.indexOf(this.tabbrowser.selectedTab); s/this.tabbrowser.selectedTab/this.selectedItem/ If the only case in which selectedIndex is -1 is when the only tab is being closed (as you described in the above comment), then visibleTabs.length would be 0, which we already check for above, so we shouldn't have to check for `selectedIndex == -1` below and the `selectedIndex < 1` should be `selectedIndex == 0`. @@ +3894,5 @@ > if (anonid == "close-button") > this.mOverCloseButton = false; > </handler> > + <handler event="mouseenter"><![CDATA[ > + let tab = event.originalTarget; Since we're using mouseenter/mouseleave, according to Dão, this should be event.target and likewise for mouseleave. @@ +3898,5 @@ > + let tab = event.originalTarget; > + if (tab.selected || tab.localName != "tab") > + return; > + > + let tabcontainer = this.parentNode; `tabcontainer` should be camel-cased as `tabContainer` for consistency with the rest of the file.

Thanks, Frank! Couple of explanations: > If the only case in which selectedIndex is -1 is when the only tab is being closed (as you described in the above comment), then visibleTabs.length would be 0, which we already check for above, so we shouldn't have to check for `selectedIndex == -1` below and the `selectedIndex < 1` should be `selectedIndex == 0`. I didn't explain this well. When the tab is closing, the new tab will be alone in |visibleTabs|, so the length is still 1. Here's the relevant portion of |_beginRemoveTab|: > 1 aTab.closing = true; > 2 this._removingTabs.push(aTab); > 3 this._visibleTabs = null; // invalidate cache > 4 if (newTab) > 5 this.addTab(BROWSER_NEW_TAB_URL, {skipAnimation: true}); So line five adds the new tab and then calls |_markTabs|, meaning |visibleTabs.length == 1| but it's not the |selectedTab| (excluded by |selectedTab.closing|). I've updated the tests to use |this.selectedItem.closing| instead of checking if the index is -1. I've also added a comment to the code that mentions the pref and the situation the test guards against. > > + let tab = event.originalTarget; > > Since we're using mouseenter/mouseleave, according to Dão, this should be event.target and likewise for mouseleave. I should have explained this. Without the tests for |originalTarget.localName != "tab"|, we would handle retargeted mouseenter/mouseleave events from anonymous content. [ tab [xul:image] [xul:label][xul:toolbarbutton] ] ^gap1 ^gap2 ^gap3 If the mouse on the |xul:image| moved right, into gap2 (margin of the |xul:image|), the tab would receive a retargeted mouseleave from the |xul:image| element. That would remove the hover attributes. We run into the same problem when hitting the other two gaps. When we get a retargeted event, the |event.target| is always the tab, but the |event.originalTarget| is whatever element it came from. For both of the handlers here, I've added comments to explain.

(In reply to Adam [:hobophobe] from comment #23) > I didn't explain this well. When the tab is closing, the new tab will be > alone in |visibleTabs|, so the length is still 1. So the closing property of the sole tab is false when _markTabs is called? visibleTabs excludes tabs with the closing property set to true: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#55 > Without the tests for > |originalTarget.localName != "tab"|, we would handle retargeted > mouseenter/mouseleave events from anonymous content. Really? Interesting. I thought that mouseenter and mouseleave don't bubble. The documentation says so: https://developer.mozilla.org/en/DOM/DOM_event_reference/mouseenter Does anonymous content not follow this?

(In reply to Frank Yan (:fryn) from comment #24) > So the closing property of the sole tab is false when _markTabs is called? > visibleTabs excludes tabs with the closing property set to true: > https://mxr.mozilla.org/mozilla-central/source/browser/base/content/ > tabbrowser.xml#55 At the time of the |_markTab| call, we have (name property for clarity): tabs: [{name: "oldtab", closing: true, selected: true}, {name: "newtab"}] The result of rebuilding |visibleTabs| is: visibleTabs: [{name: "newtab"}] > > Without the tests for > > |originalTarget.localName != "tab"|, we would handle retargeted > > mouseenter/mouseleave events from anonymous content. > > Really? Interesting. I thought that mouseenter and mouseleave don't bubble. > The documentation says so: > https://developer.mozilla.org/en/DOM/DOM_event_reference/mouseenter > Does anonymous content not follow this? The documentation on Anonymous Content explains a bit: https://developer.mozilla.org/en/XBL/XBL_1.0_Reference/Anonymous_Content#Event_Flow_and_Targeting The gist is that bubbling is different from retargeting. When anonymous content is within an element's scope, and only for certain event types (UI events), the events get retargeted to the bound element. This allows you to write a handler that would take specific actions for anonymous content without having to use JavaScript to directly attach handlers to those elements. Not sure if it has other reasons/uses. Example of that use: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/spinbuttons.xml#53

Dão, I'm relying on these attributes in bug 738491 in order to implement separators because I can't use the adjacent sibling selector due to how panorama is implemented and because the newtab-button is anonymous content. Could you take a look at this patch?

I'll review this once the theme code wanting this has stabilized, since the set of needed attributes changed multiple times in the past.

Comment on attachment 673004 [details] [diff] [review] Unbitrot > // last clicked when switching back to that tab > let focusFlags = fm.FLAG_NOSCROLL; > if (newFocusedElement && > (newFocusedElement instanceof HTMLAnchorElement || > newFocusedElement.getAttributeNS("http://www.w3.org/1999/xlink", "type") == "simple")) > focusFlags |= fm.FLAG_SHOWRING; > fm.setFocus(newBrowser, focusFlags); > } while (false); >+ this.tabContainer._markTabs(); > } > > if (!aForceUpdate) > TelemetryStopwatch.finish("FX_TAB_SWITCH_UPDATE_MS"); > ]]> > </body> This should happen unconditionally, regardless of _previewMode. > if (this._lastRelatedTab) > this._lastRelatedTab.owner = null; > else > t.owner = this.selectedTab; > this.moveTabTo(t, newTabPos); > this._lastRelatedTab = t; > } > >+ this.tabContainer._markTabs(); > return t; > ]]> > </body> Can this be moved up to where we previously set the afterselected attribute? > <![CDATA[ > Array.forEach(this.tabs, function(tab) { > if (aTabs.indexOf(tab) == -1) > this.hideTab(tab); > else > this.showTab(tab); > }, this); > >+ this.tabContainer._markTabs(); > this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false); > ]]> > </body> hideTab and showTab are part of the public API. They need to automatically update attributes rather than showOnlyTheseTabs taking care of this. > <xul:toolbarbutton class="tabs-newtab-button" > command="cmd_newNavigatorTab" > onclick="checkForMiddleClick(this, event);" >+ onmouseenter="gBrowser.mTabContainer._enterNewTab();" >+ onmouseleave="gBrowser.mTabContainer._leaveNewTab();" > tooltiptext="&newTabButton.tooltip;"/> This could be handled in the binding's mouseenter/mouseleave handlers below. >+ <method name="_markTabs"> Please rename this to _updatePositionalAttributes. (Proposals for better names welcome...) >+ <body><![CDATA[ >+ let visibleTabs = this.tabbrowser.visibleTabs; >+ >+ if (!visibleTabs.length) { >+ return; >+ } nit: remove braces

Thanks, Dão! > Please rename this to _updatePositionalAttributes. (Proposals for better names welcome...) Went with |_setPositionalAttributes|. A little shorter, and there is a |_setMenuitemAttributes| living nearby (on the binding for |tabbrowser-alltabs-popup|). Got everything else.

Comment on attachment 684909 [details] [diff] [review] Address feedback >+ <handler event="mouseenter"><![CDATA[ >+ var newtabButton = document.getAnonymousElementByAttribute( >+ this, "anonid", "tabs-newtab-button"); >+ if (event.originalTarget == newtabButton) >+ this._enterNewTab(); >+ ]]></handler> >+ >+ <handler event="mouseleave"><![CDATA[ >+ var newtabButton = document.getAnonymousElementByAttribute( >+ this, "anonid", "tabs-newtab-button"); >+ if (event.originalTarget == newtabButton) >+ this._leaveNewTab(); >+ ]]></handler> Umm, I thought there would be existing mouseenter/mouseleave event handlers where you could hook into. But since this isn't the case, putting onmouseenter and onmouseleave directly on the button is more efficient. Use document.getBindingParent(this) instead of gBrowser.mTabContainer there. >+ <handler event="mouseenter"><![CDATA[ >+ let tab = event.originalTarget; >+ // Check originalTarget type to avoid handling retargeted events >+ if (tab.selected || tab.localName != "tab") >+ return; <handler event="mouseenter" phase="target"><![CDATA[ >+ <handler event="mouseleave"><![CDATA[ >+ // Check originalTarget type to avoid handling retargeted events >+ if (event.originalTarget.localName != "tab") >+ return; ditto

The patch in bug 738491 doesn't make use of the beforeselected-visible attribute.

Thanks, Dão!

Dão, do you think you'll get to this soon? It would be nice to get this landed on m-c so we don't have to keep maintaining the patch on this bug and in the UX branch for Australis.

Yes, I intend to get back to this very soon. I haven't forgot about it, it just got pushed back by other stuff.

Comment on attachment 725668 [details] [diff] [review] Unbitrot >+ // selectedItem will not be in visibleTabs briefly when >+ // browser.tabs.closeWindowWithLastTab is disabled and the user closes >+ // the last tab >+ if (!this.selectedItem.closing && selectedIndex != 0) { >+ let beforeSelectedTab = visibleTabs[selectedIndex - 1]; >+ if (beforeSelectedTab.hasAttribute("beforehovered")) >+ beforeSelectedTab.removeAttribute("beforehovered"); >+ } Remove |if (beforeSelectedTab.hasAttribute("beforehovered"))| and just remove the attribute unconditionally. >+ if (this._afterSelectedTab.hasAttribute("afterhovered")) >+ this._afterSelectedTab.removeAttribute("afterhovered"); ditto >+ testAttrib(gBrowser.tabs[0], "first-visible-tab", true, >+ "First tab is not first-visible-tab!"); Test messages should describe the expected behavior, e.g. "First tab should have the first-visible-tab attribute", such that they make sense when the test passes as well as when it fails. Please adjust the messages in this test accordingly. >+ gBrowser.hideTab(gBrowser.tabs[1]); >+ gBrowser.mTabContainer._setPositionalAttributes(); You should never have to call _setPositionalAttributes in this test.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9ce715df8328