Invalid titles in win7 taskbar

RESOLVED FIXED in Future

Status

Firefox Graveyard
Panorama
P3
normal
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: gadjo, Unassigned)

Tracking

Trunk
Future
x86_64
Windows 7
Dependency tree / graph

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101101 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101101 Firefox/4.0b8pre

After hovering Fx icon in win7 task bar tabs thumbnails are displayed. Thumbnails titles consist of panorama group name and page title, however the first part (group name) sometimes doesn't match the real group name

Reproducible: Always

Steps to Reproduce:
1. Open some tabs in two panorama groups
2. Give names to both panorama groups
3. Hover on Fx icon in task bar
Actual Results:  
Some tabs have in the title the name of the group which they don't belong to

Expected Results:  
Group name in thumbnails titles should match the panorama group name
(Reporter)

Comment 1

7 years ago
Created attachment 487652 [details]
Visualization of the bug
(Reporter)

Updated

7 years ago
Blocks: 597043
Version: unspecified → Trunk

Comment 2

7 years ago
We've got a number of Aero Peek related questions to answer. Moving this bug to be more in line with those.
Blocks: 598154
No longer blocks: 597043
OS: Windows 7 → Windows XP
Priority: -- → P3
Blocks: 586556

Updated

7 years ago
OS: Windows XP → Windows 7

Comment 3

7 years ago
Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101110 Firefox/4.0b8pre

Able to reproduce. When hovering on the taskbar thumbnails group names does not match the panorama group name.
This issue reappeared on Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101212 Firefox/4.0b8pre . Due to this, setting this issue to NEW
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

7 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 6

7 years ago
bugspam (removing b9)
No longer blocks: 598154

Comment 7

7 years ago
Punt
No longer blocks: 608028
OS: Windows 7 → Windows XP
Target Milestone: --- → Future
Kevin, you're wrongly changing the OS field with regularity.
OS: Windows XP → Windows 7

Comment 9

6 years ago
(In reply to comment #8)
> Kevin, you're wrongly changing the OS field with regularity.

Odd... I'm not manually setting it when changing other fields. I'll keep an eye out. Sorry, and thank you.

Comment 10

6 years ago
This should be fixed by bug 581726, I just need to get it landed.
Depends on: 581726
(Reporter)

Comment 11

6 years ago
Doesn't seem to be fixed in 2011-02-06 nightly
Created attachment 555327 [details] [diff] [review]
WIP

This is probably caused by getWindowTitleForBrowser().  The group name is not returned correctly.

http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#295
Created attachment 555672 [details] [diff] [review]
WIP2
Attachment #555327 - Attachment is obsolete: true
Created attachment 556615 [details] [diff] [review]
v1

You can try the build here
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/raymond@raysquare.com-832857be9f35/
Attachment #555672 - Attachment is obsolete: true
Attachment #556615 - Flags: review?(tim.taubert)

Updated

6 years ago
Depends on: 682996
Comment on attachment 556615 [details] [diff] [review]
v1

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

Nice work finding the cause of this error! Please address those issues and ask me and Dão for review again.

::: browser/base/content/browser-tabview.js
@@ +200,5 @@
>        self._isFrameLoading = false;
>        self._window = self._iframe.contentWindow;
>        self._setBrowserKeyHandlers();
> +#ifdef XP_WIN
> +      self._updateTaskbar();

Do we need an extra function for this? If we include it here we don't even need that this._window check.

@@ +270,5 @@
> +  // Function: getGroupNameForTab
> +  // Gets the group name for the given xul:tab.
> +  getGroupNameForTab: function TabView_getGroupNameForTab(tab) {
> +    if (!this._window)
> +      return this._lastSessionGroupName;

This does only work for getActiveGroupName() because we're saving the active group's name on shutdown. If this is used by TaskbarPreviews before Panorama is loaded we'll get into the same trouble again. But we don't have any group names if we're not loaded, yet. Maybe we don't need them - I don't know exactly when TaskbarPreview ask for the tab's title.

@@ +377,5 @@
> +  // Updates the task bar.
> +  _updateTaskbar: function TabView__updateTaskbar() {
> +    if (this._window)
> +      this._window.GroupItems.updateTaskbarPreviews();
> +  },

(see above)

::: browser/base/content/tabbrowser.xml
@@ +813,5 @@
> +              if (index != -1) {
> +                let groupName = TabView.getGroupNameForTab(this.tabs[index]);
> +                if (groupName)
> +                  newTitle = groupName + sep + newTitle;
> +              }

Looks good to me but we should ask a browser peer.

::: browser/base/content/tabview/groupitems.js
@@ +184,5 @@
>        self.$titleShield.show();
>        if (self.getTitle())
>          gTabView.firstUseExperienced = true;
>        self.save();
> +#ifdef XP_WIN

You could make GroupItems.updateTaskbarPreviews() a no-op for non-Win systems by surrounding its code with #ifdef/#endif. So we'd have less clutter in the code when reading.

@@ +2594,5 @@
>        this._updateTabBar();
>      else if (shouldShowTabView)
>        UI.showTabView();
> +
> +#ifdef XP_WIN

(see above).

@@ +2595,5 @@
>      else if (shouldShowTabView)
>        UI.showTabView();
> +
> +#ifdef XP_WIN
> +    if ("gTaskbarTabGroup" in gWindow) // AeroPeek available

We don't need that double check here as that is done in updateTaskbarTabPreview() already.

@@ +2674,5 @@
> +  // Function: updateTaskbarPreviews
> +  // Updates taskbar previews for the given groupItem.
> +  updateTaskbarPreviews: function GroupItems_updateTaskbarPreviews(groupItem) {
> +    if ("gTaskbarTabGroup" in gWindow) { // AeroPeek available
> +      let updatePreviews = function updatePreviews(group) {

We don't need that "let updatePreviews" here. For strict mode compat you'd need to move that function definition out of the if() block.

@@ +2684,5 @@
> +        updatePreviews(groupItem);
> +      } else {
> +        this.groupItems.forEach(function(groupItem) {
> +          updatePreviews(groupItem);
> +        });

Better: this.groupItems.forEach(updatePreviews);

::: browser/base/content/tabview/tabitems.js
@@ +1168,5 @@
> +#ifdef XP_WIN
> +  // ----------
> +  // Function: updateTaskbarTabPreview
> +  // Updates tab preview in the taskbar for the given xul:tab.
> +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {

I think we should attach this function ("updateTaskbarPreview") to the TabItem itself.

@@ +1170,5 @@
> +  // Function: updateTaskbarTabPreview
> +  // Updates tab preview in the taskbar for the given xul:tab.
> +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {
> +    let preview = gWindow.gTaskbarTabGroup.previewFromTab(xulTab);
> +    preview.controller.wrappedJSObject.updateTitleAndTooltip();

I don't know if that's the right API to for the TaskbarPreview to update. Let's just ask Dão for review.
Attachment #556615 - Flags: review?(tim.taubert) → review-
Created attachment 558842 [details] [diff] [review]
v2

> ::: browser/base/content/browser-tabview.js
> @@ +200,5 @@
> >        self._isFrameLoading = false;
> >        self._window = self._iframe.contentWindow;
> >        self._setBrowserKeyHandlers();
> > +#ifdef XP_WIN
> > +      self._updateTaskbar();
> 
> Do we need an extra function for this? If we include it here we don't even
> need that this._window check.

Replaced it

> 
> @@ +270,5 @@
> > +  // Function: getGroupNameForTab
> > +  // Gets the group name for the given xul:tab.
> > +  getGroupNameForTab: function TabView_getGroupNameForTab(tab) {
> > +    if (!this._window)
> > +      return this._lastSessionGroupName;
> 
> This does only work for getActiveGroupName() because we're saving the active
> group's name on shutdown. If this is used by TaskbarPreviews before Panorama
> is loaded we'll get into the same trouble again. But we don't have any group
> names if we're not loaded, yet. Maybe we don't need them - I don't know
> exactly when TaskbarPreview ask for the tab's title.

getWindowTitleForBrowser() is called by the TaskbarPreview. Check out the link
http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#296

Yes, we still have the problem before Panorama is loaded, since we would display the saved active group's name on shutdown.  Two possible solutions:
1) Don't add the saved active group's name to tab title before Panorama is loaded so both window titles and titles in TaskbarPreview don't have the group name
2) Pass additional param to the getWindowTitleForBrowser() when it's called by TaskbarPreview code, so we know when not to add the group name to tab titles

> 
> @@ +377,5 @@
> > +  // Updates the task bar.
> > +  _updateTaskbar: function TabView__updateTaskbar() {
> > +    if (this._window)
> > +      this._window.GroupItems.updateTaskbarPreviews();
> > +  },
> 
> (see above)
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +813,5 @@
> > +              if (index != -1) {
> > +                let groupName = TabView.getGroupNameForTab(this.tabs[index]);
> > +                if (groupName)
> > +                  newTitle = groupName + sep + newTitle;
> > +              }
> 
> Looks good to me but we should ask a browser peer.
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +184,5 @@
> >        self.$titleShield.show();
> >        if (self.getTitle())
> >          gTabView.firstUseExperienced = true;
> >        self.save();
> > +#ifdef XP_WIN
> 
> You could make GroupItems.updateTaskbarPreviews() a no-op for non-Win
> systems by surrounding its code with #ifdef/#endif. So we'd have less
> clutter in the code when reading.

Done

> 
> @@ +2594,5 @@
> >        this._updateTabBar();
> >      else if (shouldShowTabView)
> >        UI.showTabView();
> > +
> > +#ifdef XP_WIN
> 
> (see above).
> 
> @@ +2595,5 @@
> >      else if (shouldShowTabView)
> >        UI.showTabView();
> > +
> > +#ifdef XP_WIN
> > +    if ("gTaskbarTabGroup" in gWindow) // AeroPeek available
> 
> We don't need that double check here as that is done in
> updateTaskbarTabPreview() already.
> 
> @@ +2674,5 @@
> > +  // Function: updateTaskbarPreviews
> > +  // Updates taskbar previews for the given groupItem.
> > +  updateTaskbarPreviews: function GroupItems_updateTaskbarPreviews(groupItem) {
> > +    if ("gTaskbarTabGroup" in gWindow) { // AeroPeek available
> > +      let updatePreviews = function updatePreviews(group) {
> 
> We don't need that "let updatePreviews" here. For strict mode compat you'd
> need to move that function definition out of the if() block.

Moved

> 
> @@ +2684,5 @@
> > +        updatePreviews(groupItem);
> > +      } else {
> > +        this.groupItems.forEach(function(groupItem) {
> > +          updatePreviews(groupItem);
> > +        });
> 
> Better: this.groupItems.forEach(updatePreviews);

Fixed

> 
> ::: browser/base/content/tabview/tabitems.js
> @@ +1168,5 @@
> > +#ifdef XP_WIN
> > +  // ----------
> > +  // Function: updateTaskbarTabPreview
> > +  // Updates tab preview in the taskbar for the given xul:tab.
> > +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {
> 
> I think we should attach this function ("updateTaskbarPreview") to the
> TabItem itself.

Moved

> 
> @@ +1170,5 @@
> > +  // Function: updateTaskbarTabPreview
> > +  // Updates tab preview in the taskbar for the given xul:tab.
> > +  updateTaskbarTabPreview: function TabItems_updateTaskbarTabPreview(xulTab) {
> > +    let preview = gWindow.gTaskbarTabGroup.previewFromTab(xulTab);
> > +    preview.controller.wrappedJSObject.updateTitleAndTooltip();
> 
> I don't know if that's the right API to for the TaskbarPreview to update.
> Let's just ask Dão for review.
Attachment #556615 - Attachment is obsolete: true
Attachment #558842 - Flags: feedback?(tim.taubert)
Attachment #558842 - Flags: feedback?(dao)
Comment on attachment 558842 [details] [diff] [review]
v2

I think we should get rid of this code by fixing bug 682996.
Attachment #558842 - Attachment is patch: true
Attachment #558842 - Flags: feedback?(dao) → feedback-
Comment on attachment 558842 [details] [diff] [review]
v2

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

(In reply to Dão Gottwald [:dao] from comment #17)
> I think we should get rid of this code by fixing bug 682996.

Though I'd also favor that solution we should wait for some feedback from the UX team about the change in bug 682996. Anyway, this patch fixes the described problem but we should put it on hold until we know what to do.
Attachment #558842 - Flags: feedback?(tim.taubert)
Fixed by bug 682996.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.