264812 - Text under back and forward buttons is not grayed out

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Julian Pellico]

Open a new browser window in Camino. The Back and Forward icons will be dimmed,
but the text beneath them will not. This persists whenever these icons are
enabled and disabled again.

Comment 1

[Jasper]

Haha I saw this one long time ago but never filed a bug, good catch.

Comment 2

[Julian Pellico]

I have also seen the text grayed out when the icons are enabled! Crrrrrazy!

Comment 3

[Bruce Davidson]

Accepting this bug, patch coming up.

Comment 4

[Bruce Davidson]

Attachment: Proposed patch

This patch fixes the enabled/disabled state of the text under the back and
forward buttons. We just need to call setEnabled: on the entire toolbar button
rather than just the view (the icon bit that provides the drop-downness).

Requesting r= from someone prior to requesting sr= from pinkerton.

Comment 5

[Josh Aas]

Calling setEnabled on the view alone looks pretty explicit in the existing code.
Do you know why the author might have done that? This will need a lot of
testing, perhaps especially with regards to context menu history.

Comment 6

[Bruce Davidson]

Yeah, I had that concern too. I don't know why it was written like this.

The testing I've done so far can be summarised as:

(back/forward possible) 00    01    10    11
icon & text             okay  okay  okay  okay (previously text was wrong)
icon only               okay  okay  okay  okay (unchanged)
text only               wrong wrong wrong okay (unchanged)
context menu            okay  okay  okay  okay (unchanged)
menu bar                okay  okay  okay  okay (unchanged)

With a text only toolbar the two toolbar items are always enabled both before
and after applying the patch. (Thanks for your comment, which prompted that
test.) I'll have another look tomorrow to see if I can see why that is.

I suspect icon & text is much more common, and as the current inconsistency
between the icon and text state is quite noticeable and confusing. If its not
obvious it might be better to spin that bit off to a separate bug.

Comment 7

[Julian Pellico]

While you're at it, could you fix it so that clicking on the text when the icon
is enabled, works like it does for normal toolbar buttons? ;-)

Comment 8

[Bruce Davidson]

Comment on attachment 163196 [details] [diff] [review]
Proposed patch

Clearing the review flag for this patch based on latest comment. Enabling the
text only to ignore the user click on it is a Bad Thing (tm). I'll try to have
a look at the weekend to see if I can get a patch that fixes this more
completely.

Comment 9

[Julian Pellico]

I don't think it's a negative consequence of the patch. I do think that we
should expand this bug to any deficiencies in the behavior of the back/forward
buttons. Before, label always enabled but you can never click on the label.
Desired: label is enabled => you can click on it. Shouldn't be much more work ;-)

Comment 10

[Bruce Davidson]

Turns out that smfr checked this fix in in v1.165 of BrowserWindowController.mm,
whilst addressing another problem.

Resolving as fixed. (The "always enabled in text only mode" appears to affect
other apps too, e.g. XCode, so is probably not a Camino problem.)