323761 - default page style not available when preferred stylesheet's media is not screen or all

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jonathan Haas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060104 Firefox/1.6a1 - Build ID: 2006010405

The given URL has some default styles and an alternate stylesheet for handhelds

When filling the "View->Page Style" menu Firefox thinks, he doesn't have to show the entry "default page style" because there is a preferred stylesheet set (a stylesheet with title attribute).

Unfortunately the preferred stylesheet has media="handheld" so it is not displayed in the menu and Firefox isn't displaying this the menu, too.

Reproducible: Always

Steps to Reproduce:
1. Load given URL
2. Open menu View->Page Style
Actual Results:  
There is only an entry for "No Style" if you select "No Style", you can't switch back

Expected Results:  
There should be the menuitem "Default Style"

The problem is in "function stylesheetFillPopup(menuPopup)" in browser.js

Here is a fixed version of the function, i've commented by changes:
---------------------

function stylesheetFillPopup(menuPopup)
{
  var noStyle = menuPopup.firstChild;
  var persistentOnly = noStyle.nextSibling;
  var sep = persistentOnly.nextSibling;
  while (sep.nextSibling)
    menuPopup.removeChild(sep.nextSibling);

  var styleSheets = getAllStyleSheets(window.content);
  var currentStyleSheets = [];
  var styleDisabled = getMarkupDocumentViewer().authorStyleDisabled;
  var haveAltSheets = false;
  var altStyleSelected = false;
  var altSheetsAdded = false;
  for (var i = 0; i < styleSheets.length; ++i) {
    var currentStyleSheet = styleSheets[i];

    // Skip any stylesheets that don't match the screen media type.
    var media = currentStyleSheet.media.mediaText.toLowerCase();
    if (media && (media.indexOf("screen") == -1) && (media.indexOf("all") == -1))
        continue;

    if (currentStyleSheet.title) {
      if (!currentStyleSheet.disabled)
        altStyleSelected = true;

      haveAltSheets = true;

      var lastWithSameTitle = null;
      if (currentStyleSheet.title in currentStyleSheets)
        lastWithSameTitle = currentStyleSheets[currentStyleSheet.title];

      if (!lastWithSameTitle) {
        var menuItem = document.createElement("menuitem");
        menuItem.setAttribute("type", "radio");
        menuItem.setAttribute("label", currentStyleSheet.title);
        menuItem.setAttribute("data", currentStyleSheet.title);
        menuItem.setAttribute("checked", !currentStyleSheet.disabled && !styleDisabled);
        menuPopup.appendChild(menuItem);
	altSheetsAdded = true;
        currentStyleSheets[currentStyleSheet.title] = menuItem;
      } else {
        if (currentStyleSheet.disabled)
          lastWithSameTitle.removeAttribute("checked");
      }
    }
  }

  noStyle.setAttribute("checked", styleDisabled);
  persistentOnly.setAttribute("checked", !altStyleSelected && !styleDisabled);
  persistentOnly.hidden = (window.content.document.preferredStylesheetSet && altSheetsAdded) ? true : false;
  sep.hidden = (noStyle.hidden && persistentOnly.hidden) || !haveAltSheets;
  return true;
}

------------------

I've added the var "altSheetsAdded" that checks if we have added an additional menuitem for the preferred Stylesheet. The "Default Page Style"-menuitem may only be hidden, if this is true.

Comment 1

[Jonathan Haas]

The additional variable in my patch may be not needed. It should be sufficient change the line from

persistentOnly.hidden = (window.content.document.preferredStylesheetSet) ? true : false;

to

persistentOnly.hidden = (window.content.document.preferredStylesheetSet && haveAltSheets) ? true : false;

Comment 2

[Jonathan Haas]

Better patch may be:

persistentOnly.hidden = (window.content.document.preferredStylesheetSet) ? haveAltSheets : false;

Comment 3

[PikeUK]

Jonathan, if you attach your patch to this bug you can request a review, otherwise it will probably sit here unseen.

http://www.mozilla.org/hacking/life-cycle.html
http://www.mozilla.org/projects/firefox/review.html

Comment 4

[Hermann Schwab]

(In reply to comment #3)
> Jonathan, if you attach your patch to this bug you can request a review,
> otherwise it will probably sit here unseen.
> 
> http://www.mozilla.org/hacking/life-cycle.html
> http://www.mozilla.org/projects/firefox/review.html

You've forgotten the most important link from Fix Bugs at http://www.mozilla.org/contribute/ i.e. the link to Patch Maker: http://www.gerv.net/software/patch-maker/build-mode.html

Comment 5

[Jonathan Haas]

Attachment: Patch

Here is my patch.

I don't know if this works properly, because it's my first patch an i had some problems with the Patch Maker.

Comment 6

[PikeUK]

Jonathan, you don't need to request "superreview?" for Firefox-only patches, just "review?".

You also need to pick a reviewer, the Firefox reviewers are listed at the bottom of the following page. Pick one then click "Edit" next to your patch and enter your chosen reviewer's e-mail address into the requestee field:

http://www.mozilla.org/projects/firefox/review.html

Comment 7

[Jonathan Haas]

Comment on attachment 212521 [details] [diff] [review]
Patch

Thanks for your help. I hope this is better now.

Comment 8

[Jonathan Haas]

Attachment: Patch v. 2

I suppose that the path of the browser.js in my first patch is not correct.

This patch should work better.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Is there a testcase for this bug that still works? It appears as though the site in the URL field has changed and can no longer be used to reproduce the bug.

Comment 10

[Jonathan Haas]

Attachment: small testcase

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Thanks for the testcase, Jonathan. The patch does look correct to me.

Comment 12

[Jonathan Haas]

It would also be possible (and perhaps cleaner) to set the preferred stylesheet only, if it has media="screen" or media="all", but I do not know if this would confirm with the specs or if this is easy to implement.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk and the 1.8 branch for Firefox 2.
mozilla/browser/base/content/browser.js 	1.479.2.89
mozilla/browser/base/content/browser.js 	1.585

Comment 14

[Frank Yan (:fryn)]

i noticed this last year sometime, but didn't think to file a bug for it.

glad it's fixed. :)