The Safari bug that never was (2022)
obyford.com
obyford.com
It isn't just Apple of course. I encountered almost the exact same bugs working on Nautilus at Eazel and various projects at Be. And in PowerPlant, and the Think Class Library and MacApp and... the list goes on.
My current music notation project pushed me to try and make multiple font support and layout as robust as possible, but I keep getting burnt! Text layout and metrics is just a difficult area.
It’s as “human” a problem as dates and time, probably worse! Your best bet is to use libraries to avoid the whole problem, but that means delegating all layout which might not be possible.
Hardware improves every month, but some parts of software worked better thirty years ago.
> "Also, this is a fantastic bug report. I don't know if I've ever seen a bug report this detailed before."
Little things like that restore faith in humanity.
It's about providing 10 times the value, not churning similar quality code 10 times faster.
Many cultures are very adverse against any kind of such boasting and cultivate modesty. UK is one of those countries where bragging is widely considered vulgar. Happens, but it's not considered or taken as the norm or polite.
I think this article actually perfectly sums English bragging: it's polite and short, hidden between describing the due diligence that went behind reporting the bug and the numbers of other bugs they have reported and helped fix in time combined with a semi patronizing (but absolutely true and sharable) link to how to report bugs well.
If you went further east to Asia, to cultures where any kind of boasting or bragging is seen very negatively, not just vulgar, you would've found even less of it.
This was on a Wednesday. I rolled up my sleeves to start learning WebKit development myself, but they had a patch submitted, reviewed, and accepted by the next day :-)
WebKit is open-source, so it is actually one of the few Apple projects where the developers do not not remain nameless (the other big one being Swift).
> the black hole that's called Radar
IIRC WebKit has its own bug tracker, hence why you get better response times. Radar itself is still a terrible and demoralizing experience (from a developer point-of-view)
The funny thing though is that Apple still sends WebKit bugs into Radar to work on them:
bugs.webkit.org is literally a bug tracker.
The only relevant difference between Radar and WebKit Bugzilla is private vs. public.
B.w.o is where bug fixes happen, no more. Code dev and review has to happen in b.w.o by the webkit community rules, not radar.
I assume internally they’re treated as one component which makes it easier to mirror bugs into radar?
No, this is not true. Radar is not just "a bug tracker" — it is a very powerful project management tool, custom-built over several decades for the way Apple works, with integrations into other internal tools and processes.
It should be obvious why Apple wants to track work being done on Webkit using the same tool it uses to track work being done on every other project at the company.
They closed the ticket a few days later with a picture showing the issue resolved.
There are many reasons for linking bugzilla bugs to radar, but the big ones are scheduling, other project dependencies inside Apple, security reports (so that reporters can be mentioned in release notes, etc).
The important thing is that the b.w.o->radar cross referencing is not for development specifically.
What about discussion?
Do you claim there is no private discussion among Apple engineers about WebKit bugs that have been put "InRadar"?
I see a lot of WebKit bugs that have no discussion whatsoever and just seem to arrive ex nihilo from Radar. You wonder what the reasoning or explanation is for a certain code change, and you'll find absolutely nothing about it in the Bugzilla.
Open source bugs will have private/non-public conversations (whether it's in person, or on slack, or on a seperate issue tracker) all the time. That's not abnormal or all that damning.
Is achieved by talking among each other, or simply by knowing how to fix/implement the feature (not every bug requires any discussion at all). Either in person (which would be inherently "private" as you say), or irc historically - I'm unsure whether irc is still significant as I recall free node dying off or something a few years back?
> Do you claim there is no private discussion among Apple engineers about WebKit bugs that have been put "InRadar"?
No, I'm saying your claim that "The funny thing though is that Apple still sends WebKit bugs into Radar to work on them:" is nonsense. The reason that bugs are cloned to radar is so that they can be tracked for releases. I think you have decided that any time you don't see discussion in b.w.o it means discussion there must be discussion in radar, and that's false. The reality is that most bug fix/feature implementation doesn't require discussion during development, actual recorded discussion happens during review as that systematically results in recording the exchanges.
> I see a lot of WebKit bugs that have no discussion whatsoever and just seem to arrive ex nihilo from Radar.
Again, I think you grossly overestimate the amount of recorded "discussion" that goes with a bug.
> You wonder what the reasoning or explanation is for a certain code change, and you'll find absolutely nothing about it in the Bugzilla.
For many there's not anything in radar either, because many (most?) bugs you see are created to track reviewing the change, and don't exist before the review process. The lack of explanation is typically because the engineers working on changes have the context for why a change is needed, so don't include an explanation.
Yeah, a kind of hostile takeover occurred, which the IRC community did not appreciate.
> Again, I think you grossly overestimate the amount of recorded "discussion" that goes with a bug.
Perhaps so. Seems a bit odd to me though. And a bit lonely :-)
> The lack of explanation is typically because the engineers working on changes have the context for why a change is needed, so don't include an explanation.
Well, I think it's worth noting that this is not very helpful to outsiders who are looking at and working with the code of an open source project.
Even in my closed source self owned projects, I like to provide useful context to my future self. ;-)
People communicate in IRC (or a public slack now based on another comment?), in person, or in email. So when needed communication happens, it's just generally back and forth commentary in a bug is not a particularly good or efficient way to discuss implementation of bug fixes, features, etc.
> Well, I think it's worth noting that this is not very helpful to outsiders who are looking at and working with the code of an open source project.
Because it's a bug tracker being used to track bug fixes, not a tool for communicating with arbitrary people not involved in the project? I don't mean to be glib, but for WK at least that's just not what the bug trackers are used for.
> Even in my closed source self owned projects, I like to provide useful context to my future self.
In the bug report or in the code? Generally IME when code does things that are odd the explanation of that oddness is in the code (assuming not self explanatory) not the bug tracker.
It also depends on the bug itself, bug fixes you'd normally be able to see why a change was made (if not obvious from the title/description) is the attached test cases.
I disagree, because whether it's the intention or not ("it's a bug tracker being used to track bug fixes, not a tool for communicating with arbitrary people not involved in the project"), the bug report de facto becomes an important historical record for understanding the code. In the future, when someone looks back, the question that often comes to mind is "Why?" Why this (that)? Why now (then)? The motive, the historical context, is rarely something that becomes evident just looking at the code, even code with comments.
Can I change this code? Can I delete this code? How did this code ever work? You can't rely on unit tests to tell you the story behind the code. And when you come upon some "questionable" code, it's equally likely that the comments and/or tests are questionable too.
If the code was written to handle something publicly important, if debates were had, alternatives considered, choices made and rejected, that's worth knowing, in my opinion.
Part of which is because the engineers are working on an unreleased feature that they’d like to put up “quietly”.
Afaik Apple runs really small teams versus other companies. I know when I once interviewed around FAANG, the Apple teams were significantly smaller (like 5x smaller in some cases…probably also why they aren’t part of this mass tech layoffs). I used to find in my old film job that no matter how much I churned through tickets , there was just no getting out from the stigma of being unresponsive because the person on the other end of the ticket can’t see what your workload and priorities are like. That’s not to say Apple isn’t at fault for having small teams, but I think a lot of people assume the engineers themselves don’t care to fix stuff.
Though at least in my case, I could tell my users that I was working on stuff and to expect it at some time in the future. I assume Apple’s engineers are even more hamstrung by their extreme secrecy. I know the few feedback radars I’ve gotten responses from were basically “can reproduce” to “fixed in the latest OS release” without any feedback in between.
anyway all that is to say, I often see people blame the engineers (you didn’t but I see others do) whereas I think it’s a corporate culture issue that engineers are beholden to.
Wait, you get "can reproduce" responses???
I never get that, I think literally never in over 15 years, but it would be a vast improvement to my bug reporting experience.
1. Submit bug 2. Get a cannot reproduce , please provide more information 3. Submit more info as requested 4. “Okay we can reproduce. Thanks” 5. Nothing for ages 6. Okay it’s fixed
Step number 5 almost always aligns with their OS release (point and major) schedules in my experience. macOS 13.1 just came out so if I filed a minor bug now, I’d probably not hear anything till June (WWDC) or if I’m lucky, in one of the point releases that happens every couple months.
[…]
One of them incorrectly used the width of the new line character in its calculations, and so the box that it made was too small.”
I don’t understand that explanation. If the newline character in the font is wider than a space, why would the box end up too small if width calculations assume a newline will be rendered where the actual page will use a space?
(full bug report is here: https://github.com/alphagov/reported-bugs/issues/66, but I don’t think see it linked with a WebKit issue)
https://bugs.webkit.org/show_bug.cgi?id=232939
Not sure if that helps answer your question though
> I think I see. There's a block at the bottom of RenderText::computePreferredLogicalWidths() which is only hit if wordLen == 0 and !isNewline (which is the case here) which measures the width of the one character at index i and adds it to currMaxWidth. In our situation, that's the leading newline character.
> Then, later, we subtract it out like this: "widths.max -= font.width(RenderBlock::constructTextRun(&space, 1, style));". This expects that the width of this initial character is equal to the width of a space.
The process that actually laid out the text was using the specified width of the new line, so it came up with a longer width and had to break the line.
“In HTML, any extra whitespace between words is ignored. This means you can add extra spaces, and even new lines, between words without affecting how they are displayed in the browser”
So, “ignoring the new line and just assuming it was the same width as a space” is correct behaviour.
I now think what happened is this:
- the width of the box to render the text in was computed correctly
- the code that then laid out the text inside that box incorrectly used the width of the newline in the HTML to compute the length of the text, concluded the text was too long to fit on a line, and line-wrapped it inside the box.
That’s not consistent with the claim from the article “and so the box that it made was too small”, though.
Alternatively, the font’s line feed glyph was _less_wide_ than a space, and the width of the box was computed as too small, but then the article is incorrect in that it claims the font’s line feed glyph was _wider_ than a space.
(and for those wondering why a browser needs to compute the width of that text twice: I wouldn’t know, but browser layout engines are complex beasts)
That's not true?
If it's a newline following whitespace then it should be ignored; the width should be calculated as zero, not the width of a space.
On the other hand, if it's not following whitespace the width should be that of the newline character, again not the width of a space.
Or am I misunderstanding you?
Your first guess makes perfect sense though, so perhaps that’s what happened. As I said, that was how I had put it together in my mind (regardless of actual fact).
I agree it makes it sounds like the box sizing was wrong when it must be the other way around.
I likewise have been involved in a similar bizarre bug. We wrote up a very detailed report with a reduced test case but sadly it has not received much attention. https://bugs.webkit.org/show_bug.cgi?id=247020
Just like in IE, you need to make silly workarounds.
to think, such amazing work from government. at least in US we tend to think these are not the most highly skilled people
In the US the equivalent maxes out at barely entry level salaries for private industry.
> Reviewed by NOBODY (OOPS!).
If I was reviewing the code, I would have asked why Myles changed WidthIterator.cpp's call to charactersTreatedAsSpace.constructAndAppend from a separate parameter on each line to everything inline.
This change made the diff of this change harder to read, and will make the diff of any future parameter change harder to read also.
I also would have created an intermediary variable or a helper method for `character == tabCharacter ? width : font.spaceWidth()` - it's not clear at all here why this check needs to be there (without looking at the ChangeLog, which I shouldn't have to do to understand code), or why the old behaviour that used `width` instead of `font.spaceWidth()` ONLY applies to tabCharacter (So it should be encapsulate inside a method with a clear comment with the info from the ChangeLog)
Lastly, this diff doesn't seem to match the blog description:
> The font also has data about the newline character, including its width. This doesn’t really make sense – new lines don’t (or at least shouldn’t) take up any space, but the font doesn’t treat it differently to any other character. The creator of the font still has to include a width for the new line character in the font’s data. And in the font we use on GOV.UK, the new line character has a bigger width than a space character – which is apparently unusual.
This makes it sound like the FONT is wrong and the font width shouldn't be used for width calculation. Yet the change in the code introduces using the font's space width explicitly.
Ultimately the ChangeLog doesn't match the code. I trust the blog writer, the code author, and the description of the bug in the ChangeLog. What needed improvement here then is the code to imperatively match what is described in English.
Teams that standardise on in-depth change messages usually have a culture to look towards commit messages/changelogs to understand more motivation about why code is a particularly way.
> This makes it sound like the FONT is wrong and the font width shouldn't be used for width calculation. Yet the change in the code introduces using the font's space width explicitly.
I don't know C++, and I'm not familiar with the codebase, but my read of the code change says that "if it's a 'treat-it-like-a-space-character" that isn't a tab, then size the character like a space character". This is inline with the english description
The font doesn’t have one width, it has a width for each character. The data for the newline character is bogus. All of these match.
> And in the font we use on GOV.UK, the new line character has a bigger width than a space character – which is apparently unusual.
Underfull \hbox (badness 10000)