Closed Bug 323761 Opened 19 years ago Closed 19 years ago

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

Categories

(Firefox :: General, defect)

2.0 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 2 alpha1

People

(Reporter: jonathan_haas, Assigned: jonathan_haas)

References

()

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

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.
Blocks: altss
Summary: default page style not available when preferred stylesheets media is not screen or all → [AltSS] default page style not available when preferred stylesheets media is not screen or all
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;
Better patch may be: persistentOnly.hidden = (window.content.document.preferredStylesheetSet) ? haveAltSheets : false;
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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
Attached patch Patch (obsolete) (deleted) — Splinter Review
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.
Attachment #212521 - Flags: superreview?
Attachment #212521 - Flags: review?
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 on attachment 212521 [details] [diff] [review] Patch Thanks for your help. I hope this is better now.
Attachment #212521 - Flags: superreview?
Attachment #212521 - Flags: review?(mconnor)
Attachment #212521 - Flags: review?
Attached patch Patch v. 2 (deleted) — Splinter Review
I suppose that the path of the browser.js in my first patch is not correct. This patch should work better.
Attachment #212521 - Attachment is obsolete: true
Attachment #212601 - Flags: review?(mconnor)
Attachment #212521 - Flags: review?(mconnor)
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.
Attached file small testcase (deleted) —
Thanks for the testcase, Jonathan. The patch does look correct to me.
Assignee: nobody → jonathan_haas
Hardware: PC → All
Target Milestone: --- → Firefox 2 alpha1
Version: unspecified → 2.0 Branch
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.
Attachment #212601 - Flags: review?(mconnor) → review+
Whiteboard: [checkin needed]
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
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Summary: [AltSS] default page style not available when preferred stylesheets media is not screen or all → default page style not available when preferred stylesheet's media is not screen or all
Whiteboard: [checkin needed]
i noticed this last year sometime, but didn't think to file a bug for it. glad it's fixed. :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: