467146 - font-size text attribute should be exposed in pt units

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

font-size text attribute should be exposed in pt units, not in px units.

Comment 1

[Ocie Hudson]

Since "pt units" are only meaningful for printing, not for electronic display screens, font size should be translated from "px units" to "pt units" when the text is displayed by File > Print Preview and by File > Print.  Just my opinion.

Comment 2

[Ocie Hudson]

(In reply to comment #1)
> Since "pt units" are only meaningful for printing, not for electronic display
> screens, font size should be translated from "px units" to "pt units" when the
> text is displayed by File > Print Preview and by File > Print.  Just my
> opinion.

Upon further consideration:  what you are really asking is for Firefox to behave as a What-You-See-Is-What-You-Get (WYSIWYG "wizzi-wig") document editor, like Microsoft Word or Open Office Writer, at least when Firefox displays a web page or another .HTML file which is to be printed as though it were a document.  

From what I can recall about "editors" after the introduction of Windows 3.1 and 3.2, developing WYSIWYG software is not a trivial task (the matter of software patents aside).  When you really need to see what the web page or other .HTML content that is displayed by Firefox will look like when it is printed, and control the text output by specifying the point size for each font, then maybe you should use File > Save Page as PDF, then use a "word processor" that can read the .PDF format to edit the file before you print it.

Comment 3

[Marco Zehe (:MarcoZ)]

Aaron, can you give a little more background as to why, when designing the IAccessible2 and AT-SPI APIs, it was decided that these point sizes should be in pt, not px? Comments 1 and 2 raise a few valid points here, IMO.

Comment 4

[alexander :surkov (:asurkov)]

Ocie, I'm not sure we talk about the same thing. We expose 'font-size' as text
attribute in accessibilty API (for IAccessible2 and ATK). IAccessible2 team
requested to expose 'font-size' text attribute in 'pt' units. I quite not sure
why they want to have it in 'pt' units. Concerning your previous post ('pt'
units don't make sense for displays) I'll ask them about this. Possibly all
they want is AT (screen readers) should have unique unit for 'font-size'
attribute and it doesn't matter what is the unit exactly (I keep in mind the
case when AT need to compare font sizes (bigger/smaller) and don't need to know
what this means exactly).

Comment 5

[Ocie Hudson]

Alexander, the following is from Bug 465237 which I posted about the effects of using the Firefox "Minimum font size" feature (accessed with Tools menu > Options > Content - Fonts & Colors - Advanced > Fonts dialog):

--- Comment #6 from Mardeg <[e-mail]>  2008-11-30 01:31:37 PST ---
>From the Accessibility Extension report on avg.com: http://tinyurl.com/avgpage
Violoation: Set font sizes using relative measurements (not px, pt, in, cm,
mm). 
Occurences: 190
Class: Text Sizing

This is a valid Tech Evangelism bug, AVG needs to fix the code on it's page to
make it more accessible to things like a larger minimum font size.

>From your report "some pages on any given website may not be adversely affected
while others are" you will likely find the ones that are make similar coding
mistakes, and only the developers of those sites can fix that, sorry.

Here's the accessibility extension:
https://addons.mozilla.org/en-US/firefox/addon/5809
The report was generated using "beta rule set"
-------------------------------------------------------------------------------

As the "violation" message says, the web page designer/coder should use "relative measurements" and not px (pixel), pt (point size), in (inches), cm (centimeters), mm (millimeters).

Frankly, it has been many years since I worked as an analyst/programmer and I don't know anything about current "accessibility" standards or their adoption.  I don't know whether HTML 2.0 has a provision for relative sizes of letters, digits and symbols (text characters).  I realize that "relative measurements" would be an advantage in re-sizing a web page, or the display of any document, but I have no idea how "relative measurements" are created and implemented in practice.  Perhaps the IAccessible2 team is looking for a "unit" of relative size, such as a fraction or a multiple:

(a) 1000 is the largest size for a font, each smaller size is expressed as a fraction of it; for example, size 500 would be one half the size of the largest size.

(b) 100 is the smallest size for a font, each larger size is expressed as a multiple of it; for example, size 150 would be one-and-one-half larger than the smallest size, and 200 would be twice the size of the smallest size.

Note that using "1000" instead of "1.00" for the largest size, and using "100"  instead of "1.00" for the smallest size, avoids calculations with decimal fractions, so that all of the arithmetic is done with integers. :-)


That said, here is an exercise that shows me (and perhaps you, too) that with Firefox, I do not get, printed on the paper page, what I see on the display screen: 

(1) I used File > Print Preview to display the web page on which this message is displayed.  Then I measured the length of the following line, with a ruler:

> Since "pt units" are only meaningful for printing, not for electronic display

(2) On my display monitor, Print Preview displays that line as about 8.5 inches long (the width of the most commonly used paper page for printing letters and documents -- 8.5 in. x 11 in.).  The characters in that displayed text are larger than the size of the same characters, as they are displayed by Firefox for this Bugzilla page.

(3) On this Bugzilla display, the line is only about 7 inches long, and the characters are smaller in size than shown by Print Preview.  

(4) When I print the page with the HP Deskjet D1420, the same line is 6.38 inches long, but the size of the characters appears to be a little bit larger than they are displayed by Bugzilla.  

So, what I see displayed by Firefox on my computer's monitor is not the same as what I will get when the document is printed.  In contrast, a WSYIWYG document editor will display text characters the same size on the monitor as they will be when they are printed.  But as I said before, making WYSIWYG is definitely not a trivial task.

Comment 6

[alexander :surkov (:asurkov)]

Pete, could you comment?

Comment 7

[Pete Brunet]

The idea was that for all kinds of apps the screen readers would only have to deal with one kind of unit for font size and that pt was closer to how end users think of font sizes.  Aaron also commented in a separate email:

I think pt is right. First, if the AT wants px it can look at the character bounds using IAccessibleText Second, pt is usually how end users think about font sizes. It's how one picks a font size when editing in basically any application.

Comment 8

[Ocie Hudson]

Pete & Alexander,

Do you know which "unit" that Firefox 3.0.4 uses when we exercise the options on the Tools > Options > Content: Fonts & Colors (Default drop-downs) and Advanced > Fonts dialog??  

The numbers seem "relative" to me, because when I choose "16" as the "Minimum font size", the size of the characters that are displayed for Firefox menu Help > Help Contents is quite _considerably_ larger than the size of the characters that are displayed for Bugzilla messages.  But that difference in size also exists  when the "Minimum font size" is set to "None"; the Help Contents characters are much larger than the relatively tiny characters displayed for Bugzilla messages.  So, apparently the Firefox configured size is relative to the default font size chosen for the page by its designer (the option "Allow pages to choose their own fonts, ..." is enabled).

With regard to the end-users, is a specific "unit" necessary?  If you choose 
point size, then some end-users will reasonably expect the characters to appear just as they would on a printed document with that point size.  In effect, a "screen reader" then should be What-You-See-Is-What-You-Get (WYSIWYG), which is ordinarily the case now when "editing in basically any application" (that runs under Windows, anyway).  How feasible that would be for a "screen reader", I don't know.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Okay, I think we need to provide what the external APIs (IA2) expect for font-size units, so I think we must do a conversion to pt here. I'll give it a shot.

Note ATK_TEXT_ATTR_SIZE doesn't specify units AFAIK

Comment 10

[Aaron Leventhal]

David, we don't use those outdated ATK text attribute constants. We expose exactly the same text attributes for ATK as we do in IA2. This came after discussion with Bill Haneman orignally, and later Will Walker agreed that we should use what the Linux Foundation IA2 group decided on.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Perfect. Thanks Aaron. I keep finding old ATK docs, and of course I never used the text attributes much (or at all) in GOK.

Comment 12

[alexander :surkov (:asurkov)]

David, just FYI, sometime ago I started the patch but didn't understand completely layout code needed for this. I'm not sure I will get back this to really soon, so if you feel hot on this in the meantime then do it.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

The more I dig into this, the more I agree that "pt" might be a miss-match for font-size in hypertext. I'm tempted to try and fix this in spec. Also, adding checks for unit type in nsHyperTextAccessible::GetDefaultTextAttributes might be a perf pooper.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: possible fix (no tests yet)

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 357391 [details] [diff] [review]
possible fix (no tests yet)

Marco, in manual tests this looks like it does the right conversion for various types css font-size values. I'm not sure of the right automated tests because the conversion to pt might be dependent on the device context. Maybe it would be enough just to check the last two characters of our font-size text attribute for "pt"?

Comment 16

[alexander :surkov (:asurkov)]

Few things 1) it's preferable to keep the code inside of textattr file, 2) create specific class like we have for background-color 3) share code with layout (that's why I said I didn't understand completely needed layout code for this. If we want this to fix this bug very quick then please file following up bugs and add XXX comments.

Comment 17

[Marco Zehe (:MarcoZ)]

Error output from Mochitests looks like this with the patch:
1056 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area1 - got "12pt", expected "16px"
1070 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area2 - got "12pt", expected "16px"
1091 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area3 - got "12pt", expected "16px"
1111 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area4 - got "12pt", expected "16px"
1127 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area5 - got "12pt", expected "16px"
1144 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area6 - got "12pt", expected "16px"
1173 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area7 - got "12pt", expected "16px"
1211 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area8 - got "10pt", expected "13.3333px"

As you can see, you're converting!

I also did some manual tests using both a build with your patch and one regular nightly, and the point sizes you return usually match what I see with the JAWS cursor, which is the mouse pointer emulator, and which always grabs the point size displayed on-screen.

But I agree with Alexander, you shouldn't dump this algorithm right into nsHyperTextAccessible. I'd suggest to go with the approach he suggests in comment #16.

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Surkov, Marco, this is exactly the feedback I was looking for, thanks.

(In reply to comment #16)
> Few things 1) it's preferable to keep the code inside of textattr file, 2)
> create specific class like we have for background-color 3) share code with
> layout (that's why I said I didn't understand completely needed layout code for
> this. If we want this to fix this bug very quick then please file following up
> bugs and add XXX comments.

1) Sounds good to me.
2) OK
3) I'm not sure what 'share' means here. Do you think I should create a new layout method that does the conversion?

(In reply to comment #17)
Thanks Marco, I will have to update the tests of course :)

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to comment #18)
> 3) I'm not sure what 'share' means here. Do you think I should create a new
> layout method that does the conversion?

Yes, I believe that's what Alex meant. I know he did this for some of the other text attribute things. The big patch that first introduced everything was in bug 345759.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: moved smarts into nsTextUtils, modified test_textattrs

I thought about making a static util method for px to pt conversion but in the end, I followed Surkov (and Marco's) advice and created a class akin to the background color attr stuff.

Comment 21

[Marco Zehe (:MarcoZ)]

With this, I get one failure on Windows:
1218 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area8 - got "10pt", expected "8pt"

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: Same as before but with OS sniff.

It appears font-size for inputs on Linux are 10pt, and on Mac/Windows, 8pt.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: my unended patch

possibly it may be interesting

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

I think nsFontTextAttr::Equal can just do whatever is most performant. I'll tweak it.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: combined patch

WIP, this passes the tests. Need to figure out why I needed to divide by magic 4.

Comment 26

[Marco Zehe (:MarcoZ)]

Nit:
>+            "font-size": (MAC)? "8pt" : "10pt",

Please insert a space before the ? to comply with generally used style. Thanks!

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: cleaner

I went back to using the recommended device context. Apparently TWIPs are deprecated with the full page zoom work. I have made the patch simpler though, and ::Equal comparison doesn't convert to pt anymore (better perf). We can do the conversion if ATs request it.

All tests pass for me. Marco can you test in Windows. Will be nice to close this.

Comment 28

[Aaron Leventhal]

Comment on attachment 357838 [details] [diff] [review]
cleaner

For the platform-specific stuff, I guess you should just use #ifdef XP_WIN etc.

Comment 29

[Marco Zehe (:MarcoZ)]

Comment on attachment 357838 [details] [diff] [review]
cleaner

This one works! Good job!

Comment 30

[Marco Zehe (:MarcoZ)]

Comment on attachment 357838 [details] [diff] [review]
cleaner

Requesting additional review from Alex.

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #28)
> (From update of attachment 357838 [details] [diff] [review])
> For the platform-specific stuff, I guess you should just use #ifdef XP_WIN etc.

Hi Aaron, we only really need to check the os in the tests... so I did it the JS way.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> (In reply to comment #28)
> > (From update of attachment 357838 [details] [diff] [review] [details])
> > For the platform-specific stuff, I guess you should just use #ifdef XP_WIN etc.
> 
> Hi Aaron, we only really need to check the os in the tests... so I did it the
> JS way.

I guess just Aaron meant it's preferable to use preprocessor directives, just I'm not sure if they can be used in mochitests (though iirc they can be used in any file included into makefile.in but I may be wrong).

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 357838 [details] [diff] [review]
cleaner

font-size isn't exposed as text attribute any more, only as a part of default text attributes.

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #33)
> (From update of attachment 357838 [details] [diff] [review])
> font-size isn't exposed as text attribute any more, only as a part of default
> text attributes.

It never was exposed as non-default on the ATK side AFAICT. I'll add the logic in for when the defaults flag is false for completeness assuming this makes a difference on the Windows side.

Comment 35

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: more complete, includes non-default exposure.

Comment 36

[alexander :surkov (:asurkov)]

nit: / XXX todo: consider moving to layout module - it's better "consider to share this code with layout module" or similar otherwise it sounds for me layout module doesn't have similar code. And I would like to see a XXX bug xxxxx :)

Comment 37

[alexander :surkov (:asurkov)]

nit: NS_ENSURE_TRUE(dc, NS_ERROR_UNEXPECTED); it's NS_ENSURE_STATE(dc)

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #36)
Created bug 474621
Will update patch.

Comment 39

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: addresses surkov's nits

Comment 40

[Marco Zehe (:MarcoZ)]

Comment on attachment 357985 [details] [diff] [review]
addresses surkov's nits

All tests pass on Windows as well, thanks!

Comment 41

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 357985 [details] [diff] [review]
addresses surkov's nits

Surkov, want another look?

Comment 42

[alexander :surkov (:asurkov)]

Comment on attachment 357985 [details] [diff] [review]
addresses surkov's nits


> static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
>-  // CSS name            CSS value        Attribute name              Attribute name
>+  // CSS name            CSS value        Attribute name              Attribute value

nit: what have been changed here?
>+PRBool
>+nsFontSizeTextAttr::Get(nsAString& aValue)
>+{
>+  // Do not expose "background-color" text attribute if its value is matched
>+  // with the default value.
>+  nscoord size = GetFontSize(mFrame);

nit: fontSize variable name is better?

>+  if (mRootFrame && size == GetFontSize(mRootFrame))
>+    return PR_FALSE;
>+
>+  nsAutoString value;
>+  // XXX todo: consider sharing this code with layout module (bug 474621)
>+  nsIDeviceContext *dc = mFrame->PresContext()->DeviceContext();
>+  NS_ENSURE_STATE(dc);

don't use NS_ENSURE_STATE here because your method is boolean.

>+  float inches = (double)(GetFontSize(mFrame)) / (double)dc->AppUnitsPerInch();

nit: iirc, c style cast are deprecated, it's preferable to use c++ style casts - static_cast

>+  int pts = inches * 72 + .5;

I would like to get sr for this code from Roc.

>+/**
>+ * Class is used for the work with "font-size" text attribute. It is
>+ * used in nsHyperTextAccessible.
>+ */
>+class nsFontSizeTextAttr : public nsTextAttr
>+{
>+public:
>+  nsFontSizeTextAttr(nsIFrame *aFrame, nsIFrame *aRootFrame);
>+  
>+  // nsTextAttr
>+  virtual PRBool Equal(nsIDOMElement *aElm);
>+

nit: would be nice to put here the comment // nsFontSizeTextAttr

>+  /**
>+   * @note  IA2 requires pt size as integer.

nit: technically it's not correct because this comment doesn't address needs of ATK. Any @note follows method description

>+   * 
>+   * returns font size in pts as integer.

nit: since return value is not font size then it's preferable to put right comment here. Here I would like to see comment when we return PR_TRUE and when PR_FALSE.

>+   */
>+  virtual PRBool Get(nsAString& aValue);
>+
>+private:
>+  /**
>+   * Return font size for the given frame.
>+   *
>+   *

nit: excess "*"

>+   * @param aFrame      [in] the given frame to query font-size
>+   * @return            font size
>+   */
>+   nscoord GetFontSize(nsIFrame *aFrame);
>+
>+  nsIFrame *mFrame;
>+  nsIFrame *mRootFrame;
>+  nsCOMPtr<nsIDOMCSSStyleDeclaration> mStyleDecl;

unused member

>     nsAutoString value, oldValue;
>-    if (textAttr.Get(name, value))
>+    if (textAttr.Get(name, value)) {
>       attributes->SetStringProperty(name, value, oldValue);
>+    }

nit: this change isn't necessary, my own preference do not use {} for single if statement.

>   }
>+  
>+  nsIFrame *sourceFrame = nsCoreUtils::GetFrameFor(element);
>+  NS_ENSURE_TRUE(sourceFrame, NS_ERROR_UNEXPECTED);

nit: use NS_ENSURE_STATE

>+  // font size must be exposed in pt units

nit: this comment is excess, nsHyperTextAccessible is wrong place to care about units I guess.

>+  nsAutoString value;
>+  nsFontSizeTextAttr fontSizeTextAttr(sourceFrame, nsnull);
>+  fontSizeTextAttr.Get(value);
>+  nsAccUtils::SetAccAttr(attributes,
>+                         nsAccessibilityAtoms::fontSize, value);
>+  
>+  value.Truncate(0);

nit: iirc Truncate() defines default attribute value, it's no need to pass 0.

nit: new line please

>+  // set font background color
>+  nsBackgroundTextAttr backgroundTextAttr(sourceFrame, nsnull);
>+  if (backgroundTextAttr.Get(value)) {

I think you can drop 'if' statement here like you have for font-size case because Get() method can't return false for default text attributes case. Additionally I think it makes sense to add comment to Get() method description,  

>+////////////////////////////////////////////////////////////////////////////////
>+// OS detect
>+const MAC = (navigator.platform.indexOf("Mac") != -1)? true : false;
>+const LINUX = (navigator.platform.indexOf("Linux") != -1)? true : false;
>+const WIN = (navigator.platform.indexOf("Win") != -1)? true : false;

>           gComputedStyle = document.defaultView.getComputedStyle(node, "");
>+          var fontSize;
>+          if (navigator.platform.indexOf)

what is this?


I like to look at new patch.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #42)
> (From update of attachment 357985 [details] [diff] [review])
> 
> > static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
> >-  // CSS name            CSS value        Attribute name              Attribute name
> >+  // CSS name            CSS value        Attribute name              Attribute value
> 
> nit: what have been changed here?

I changed 'Attribute name' to 'Attribute value' -- is that correct?

Comment 44

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #42)
> (From update of attachment 357985 [details] [diff] [review])
> >+  nsAutoString value;
> >+  // XXX todo: consider sharing this code with layout module (bug 474621)
> >+  nsIDeviceContext *dc = mFrame->PresContext()->DeviceContext();
> >+  NS_ENSURE_STATE(dc);
> 
> don't use NS_ENSURE_STATE here because your method is boolean.

I'm confused, please see your comment 37 ;)

Comment 45

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 357985 [details] [diff] [review] [details])
> > 
> > > static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
> > >-  // CSS name            CSS value        Attribute name              Attribute name
> > >+  // CSS name            CSS value        Attribute name              Attribute value
> > 
> > nit: what have been changed here?
> 
> I changed 'Attribute name' to 'Attribute value' -- is that correct?

you're right, sorry

Comment 46

[alexander :surkov (:asurkov)]

(In reply to comment #44)
> (In reply to comment #42)
> > (From update of attachment 357985 [details] [diff] [review] [details])
> > >+  nsAutoString value;
> > >+  // XXX todo: consider sharing this code with layout module (bug 474621)
> > >+  nsIDeviceContext *dc = mFrame->PresContext()->DeviceContext();
> > >+  NS_ENSURE_STATE(dc);
> > 
> > don't use NS_ENSURE_STATE here because your method is boolean.
> 
> I'm confused, please see your comment 37 ;)

Yea, I know, but there I said about replacement NS_ENSURE_TRUE on NS_ENSURE_SUCCESS - that was correct. But I didn't noticed return value is boolean. It means you can't use these macros there because these macros returns nsresult.

Comment 47

[David Bolter [:davidb] (NeedInfo me for attention)]

Sorry about these bits, I rushed my patch up too soon without self-review. I'm
glad you caught them!

> >+  nsCOMPtr<nsIDOMCSSStyleDeclaration> mStyleDecl;
> 
> unused member

Nice catch.

> 
> >     nsAutoString value, oldValue;
> >-    if (textAttr.Get(name, value))
> >+    if (textAttr.Get(name, value)) {
> >       attributes->SetStringProperty(name, value, oldValue);
> >+    }
> 
> nit: this change isn't necessary, my own preference do not use {} for single if
> statement.

OK. That was a left-over because I had multiple statements.

> I think you can drop 'if' statement here like you have for font-size case
> because Get() method can't return false for default text attributes case.
> Additionally I think it makes sense to add comment to Get() method description, 


> >+          if (navigator.platform.indexOf)
> 
> what is this?

Beats me! Removing.

Comment 48

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: Removed cruft, tweaked comments, improved cast etc.

Comment 49

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: (repost - with proper mime type)

Comment 50

[alexander :surkov (:asurkov)]

Comment on attachment 358183 [details] [diff] [review]
(repost - with proper mime type)


>+PRBool
>+nsFontSizeTextAttr::Get(nsAString& aValue)
>+{
>+  // Do not expose "background-color" text attribute if its value is matched
>+  // with the default value.

nit: oops, "background-color"? ;)

>+  nscoord fontsize = GetFontSize(mFrame);

nit: I like fontSize, but it's up to you.

>+  if (mRootFrame && fontsize == GetFontSize(mRootFrame))
>+    return PR_FALSE;
>+
>+  // Convert from nscoord to pt.
>+  //
>+  // Note: according to IA2, "The conversion doesn't have to be exact.
>+  // The intent is to give the user a feel for the size of the text."

nit: would be nice to have two phrases it doesn't go contrary to ATK.

> class nsBackgroundTextAttr : public nsTextAttr
> {
> public:
>   nsBackgroundTextAttr(nsIFrame *aFrame, nsIFrame *aRootFrame);
>   
>-  // nsTextAttr
>+  // nsBackgroundTextAttr
>   virtual PRBool Equal(nsIDOMElement *aElm);

nit: I meant put //nTextAttr before Equal() and put //nsBackgroundColor before Get().

>+  // nsFontSizeTextAttr
>+  virtual PRBool Equal(nsIDOMElement *aElm);

nit: the same

>+  
>+  nsIFrame *sourceFrame = nsCoreUtils::GetFrameFor(element);
>+  NS_ENSURE_TRUE(sourceFrame, NS_ERROR_UNEXPECTED);

nit: NS_ENSURE_STATE

>+    value.Truncate(0);

nit: empty argument please

>           gComputedStyle = document.defaultView.getComputedStyle(node, "");
>+          var fontSize;

nit: fontSize isn't used, right?

rest looks great, thank you for patience, r=me

Comment 51

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch to go in

Thanks to my reviewers, this is another davidb learning stuff patch :)

Marco, can you do the push?

Comment 52

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #50)
> (From update of attachment 358183 [details] [diff] [review])

> >+  // nsBackgroundTextAttr
> >   virtual PRBool Equal(nsIDOMElement *aElm);
> 
> nit: I meant put //nTextAttr before Equal() and put //nsBackgroundColor before
> Get().
> 

Just to follow up... Surkov, now this nit makes sense to me, since we are using a virtual method here!  Nice to get clarity as I didn't understand the prior related nit.

Comment 53

[Marco Zehe (:MarcoZ)]

Pushed on Davidb's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/c1c28d2c16f4

Comment 54

[Marco Zehe (:MarcoZ)]

Comment on attachment 358188 [details] [diff] [review]
patch to go in

This is needed for completion of text attribute support in accessibility, which is a new Firefox 3.1 feature.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd note that using layout's ratio to convert app units to points isn't necessarily going to be the same definition of points that other windows apps use.  Our internal DPI can be different due to the layout.css.dpi pref.  If you wanted to get a more reliable conversion to a number meaningful to other windows apps, I think you should convert to device pixels using Gecko APIs, and then convert device pixels to points based on the number returned from a call to the Win32 API GetDeviceCaps(dc, LOGPIXELSY).

Comment 56

[Marco Zehe (:MarcoZ)]

(In reply to comment #55)
> I'd note that using layout's ratio to convert app units to points isn't
> necessarily going to be the same definition of points that other windows apps
> use.  Our internal DPI can be different due to the layout.css.dpi pref.

The ones who requested that we expose PT instead of PX were indeed the people on the IAccessible2 standards group, and IA2 is Win32. However, the conversion applies to all platforms. Do you know if something similar to what you suggest below applies to Linux and Mac OS as well?
> If you wanted to get a more reliable conversion to a number meaningful to
> other windows apps, I think you should convert to device pixels using Gecko
> APIs, and then convert device pixels to points based on the number returned
> from a call to the Win32 API GetDeviceCaps(dc, LOGPIXELSY).

OK, if I got this right, the only thing we'd need to do then is to make these platform-specific calls in our platform a11y wrappers.
Again, it would be good to know if Linux and Mac also have these peculiarities.

Thanks for the suggestion! I believe we can put this adjustment into a separate bug. My manual testing showed that this mostly works accurately currently, so I don't think there's a need to back this out and retry. Rather, we can develop on top of the current solution.

Comment 57

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #55)
Great info thanks! I'll wait to see answers to Marco's x-platform question.

Comment 58

[David Bolter [:davidb] (NeedInfo me for attention)]

Actually I realize you are probably too busy to go hunting down px to pt conversions on other platforms. Note there is a comment on point conversions in the IA2 spec:
"The conversion doesn't have to be exact. The intent is to give the user a feel for the size of the text."

Comment 59

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 358188 [details] [diff] [review]
patch to go in

a191=beltzner

Comment 60

[Marco Zehe (:MarcoZ)]

Pushed to mozilla-1.9.1 on David's behalf in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/17822d168308

Comment 61

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)