299881 - Block images context menu entry does not show checkbox when on

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Sylvain Pasche]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050513 Debian/1.7.8-1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050706 Firefox/1.0+

When you right click on an image, and choose "Block images from ...", the
checkbox on the left is not checked (not visible). However, the image blocking
is in effect.
I tried with stable firefox, and the checkbox is visible when image blocking is
activated, but there is apparently a regression on Deer Park.

Reproducible: Always

Steps to Reproduce:
1. Right click an image 
2. Choose "block images from ..."
3. Right click again on the image

Actual Results:  
There's no checkbox showing that the image blocking is active

Expected Results:  
There should be a checked box on the left of the menu item

Comment 1

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

Is there a specific site or configuration needed to reproduce this? It seems to
work for me using a Windows Firefox build from 20050719.

Comment 2

[Jeff Walden [:Waldo]]

I'm hunting down the regression now.  The bug exists at least as far back as
2005-06-13.

Comment 3

[Jeff Walden [:Waldo]]

This seems to be Linux-specific based on reports here, and possibly
GTK2-specific as well based on what I've discovered.  After tracking through old
builds on archive.m.o, it seems the problem first showed up between the
2005-02-17-09 and 2005-02-18-10 builds.  Checkins during that time:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&sortby=Date&date=explicit&mindate=2005-02-17+08%3A00&maxdate=2005-02-18+10%3A00

My guess is the regression occurred with the fix for bug 174740.

Adding |type="checkbox"| where the menu item is defined is probably something
that needs doing.  I initially thought this was the bug, but I tested and it's
not - the check still doesn't display even with that added.

http://lxr.mozilla.org/mozilla/source/browser/base/content/browser-context.inc#102

Anyway, CCing the fixer for bug 174740, as he can probably figure out what's
wrong here.  Ginn, do you see the problem?

Comment 4

[Ginn Chen]

Thanks, Jeff.
I saw this problem.
I'll try to fix it in next week.

Comment 5

[Ginn Chen]

Attachment: patch

fix it, no need waiting for next week

Comment 6

[Ginn Chen]

Comment on attachment 190107 [details] [diff] [review]
patch

neil, would you please review it? Thanks!

Comment 7

[[e-mail]]

Comment on attachment 190107 [details] [diff] [review]
patch

Sorry, I can't review this because a) I don't have Firefox b) I don't have GTK2
c) I think the menuitem should have type="checkbox" d) it should be able to use
-moz-appearance without #ifdefs

Comment 8

[Ginn Chen]

(In reply to comment #7)
> c) I think the menuitem should have type="checkbox"
I agree with you.
But this patch is not try to make behaviours of gnomestripe theme or gtk2 theme
same to other platforms, as they did before patch of bug 174740.
 
> d) it should be able to use
> -moz-appearance without #ifdefs
It is not introduced in this patch.
We should solve it seperately. 

Comment 9

[Antony Gelberg]

(In reply to comment #8)
> (In reply to comment #7)
> > c) I think the menuitem should have type="checkbox"
> I agree with you.
> But this patch is not try to make behaviours of gnomestripe theme or gtk2 theme
> same to other platforms, as they did before patch of bug 174740.
>  
> > d) it should be able to use
> > -moz-appearance without #ifdefs
> It is not introduced in this patch.
> We should solve it seperately. 

Is there any direction here?  Is the patch going to be altered / re-reviewed?

Comment 10

[Ginn Chen]

Comment on attachment 190107 [details] [diff] [review]
patch

bryner, would you please review this patch or give comments? This is blocking
next release. Thanks.

Comment 11

[Ginn Chen]

I think it should be on the list of bug 300860.

Comment 12

[Ginn Chen]

Fixed on trunk

Checking in themes/classic/global/win/menu.css;
/cvsroot/mozilla/themes/classic/global/win/menu.css,v  <--  menu.css
new revision: 1.58; previous revision: 1.57
done
Checking in toolkit/themes/gnomestripe/global/menu.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/menu.css,v  <--  menu.css
new revision: 1.11; previous revision: 1.10
done

Comment 13

[Ginn Chen]

Fixed 1.8 branch

Checking in themes/classic/global/win/menu.css;
/cvsroot/mozilla/themes/classic/global/win/menu.css,v  <--  menu.css
new revision: 1.56.8.1; previous revision: 1.56
done
Checking in toolkit/themes/gnomestripe/global/menu.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/menu.css,v  <--  menu.css
new revision: 1.10.2.1; previous revision: 1.10
done